From 52d658d50e8e77ca1c92e70801b44e179cb1f647 Mon Sep 17 00:00:00 2001 From: Teemu Murtola Date: Tue, 16 Jun 2015 12:53:02 +0300 Subject: [PATCH] Improve logic for automatic selection names If a selection is directly provided from an external index group, the name of the selection is set to be the name of the group, irrespective of the exact way the group is selected. The code that did this was a bit too eager, and used the name of the group that appeared around the beginning of a more complex selection in some other cases, leading to confusion. Make the code only use the name of the group if the group is really used as-is, and otherwise use the selection text. Add explicit tests for this behavior. Change-Id: Ic7fe2626a07cfd3cac3773046ed9f83484ba4368 --- src/gromacs/selection/keywords.h | 13 ++++- src/gromacs/selection/selelem.cpp | 16 +----- src/gromacs/selection/sm_position.cpp | 13 +++++ ...ataTest_HandlesIndexGroupsInSelections.xml | 56 +++++++++++++++++++ ...HandlesUnsortedIndexGroupsInSelections.xml | 8 +-- .../selection/tests/selectioncollection.cpp | 7 ++- 6 files changed, 94 insertions(+), 19 deletions(-) diff --git a/src/gromacs/selection/keywords.h b/src/gromacs/selection/keywords.h index eb503a7e7e..6726dbb03b 100644 --- a/src/gromacs/selection/keywords.h +++ b/src/gromacs/selection/keywords.h @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2009,2010,2012,2014, by the GROMACS development team, led by + * Copyright (c) 2009,2010,2012,2014,2015, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -65,6 +65,17 @@ extern struct gmx_ana_selmethod_t sm_keyword_pos; void _gmx_selelem_print_compare_info(FILE *fp, void *data); +/*! \brief + * Returns whether the selection element is a default position keyword. + * + * \param[in] sel Selection element to query. + * \returns ``true`` if ``sel`` represents a position keyword evaluation that + * uses the default (implicit) position keyword. + * + * This method only works before the selection has been compiled. + */ +bool +_gmx_selelem_is_default_kwpos(const gmx::SelectionTreeElement &sel); /** Sets the position type for position keyword evaluation. */ void _gmx_selelem_set_kwpos_type(gmx::SelectionTreeElement *sel, const char *type); diff --git a/src/gromacs/selection/selelem.cpp b/src/gromacs/selection/selelem.cpp index 8cca62b067..64013091dc 100644 --- a/src/gromacs/selection/selelem.cpp +++ b/src/gromacs/selection/selelem.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2009,2010,2011,2012,2013,2014, by the GROMACS development team, led by + * Copyright (c) 2009,2010,2011,2012,2013,2014,2015, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -303,18 +303,8 @@ void SelectionTreeElement::fillNameIfMissing(const char *selectionText) // Check whether the actual selection given was from an external group, // and if so, use the name of the external group. SelectionTreeElementPointer child = this->child; - while (child->type == SEL_MODIFIER) - { - if (!child->child || child->child->type != SEL_SUBEXPRREF - || !child->child->child) - { - break; - } - child = child->child->child; - } - if (child->type == SEL_EXPRESSION - && child->child && child->child->type == SEL_SUBEXPRREF - && child->child->child) + if (_gmx_selelem_is_default_kwpos(*child) && child->child + && child->child->type == SEL_SUBEXPRREF && child->child->child) { if (child->child->child->type == SEL_CONST && child->child->child->v.type == GROUP_VALUE) diff --git a/src/gromacs/selection/sm_position.cpp b/src/gromacs/selection/sm_position.cpp index fe85404536..ad174aa159 100644 --- a/src/gromacs/selection/sm_position.cpp +++ b/src/gromacs/selection/sm_position.cpp @@ -225,6 +225,19 @@ set_poscoll_pos(gmx::PositionCalculationCollection *pcc, void *data) ((t_methoddata_pos *)data)->pcc = pcc; } +bool +_gmx_selelem_is_default_kwpos(const gmx::SelectionTreeElement &sel) +{ + if (sel.type != SEL_EXPRESSION || !sel.u.expr.method + || sel.u.expr.method->name != sm_keyword_pos.name) + { + return false; + } + + t_methoddata_pos *d = static_cast(sel.u.expr.mdata); + return d->type == NULL; +} + /*! * \param[in,out] sel Selection element to initialize. * \param[in] type One of the enum values acceptable for diff --git a/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesIndexGroupsInSelections.xml b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesIndexGroupsInSelections.xml index 9cd47b98bb..8cae2e62f0 100644 --- a/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesIndexGroupsInSelections.xml +++ b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesIndexGroupsInSelections.xml @@ -26,6 +26,24 @@ group "GrpB" and resname RB false + + group "GrpA" permute 5 3 2 1 4 + group "GrpA" permute 5 3 2 1 4 + group "GrpA" permute 5 3 2 1 4 + false + + + group "GrpA" plus group "GrpB" + group "GrpA" plus group "GrpB" + group "GrpA" plus group "GrpB" + false + + + res_cog of group "GrpA" + res_cog of group "GrpA" + res_cog of group "GrpA" + false + @@ -70,5 +88,43 @@ 5 + + group "GrpA" permute 5 3 2 1 4 + + 5 + 3 + 2 + 1 + 4 + 0 + + + + group "GrpA" plus group "GrpB" + + 10 + 0 + 1 + 2 + 3 + 4 + 3 + 4 + 5 + 6 + 7 + + + + res_cog of group "GrpA" + + 5 + 0 + 1 + 2 + 3 + 4 + + diff --git a/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesUnsortedIndexGroupsInSelections.xml b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesUnsortedIndexGroupsInSelections.xml index cc54442ee6..67a286d680 100644 --- a/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesUnsortedIndexGroupsInSelections.xml +++ b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesUnsortedIndexGroupsInSelections.xml @@ -25,13 +25,13 @@ res_cog of group "GrpUnsorted" - GrpUnsorted + res_cog of group "GrpUnsorted" res_cog of group "GrpUnsorted" false group "GrpUnsorted" permute 2 1 - GrpUnsorted + group "GrpUnsorted" permute 2 1 group "GrpUnsorted" permute 2 1 false @@ -287,7 +287,7 @@ - GrpUnsorted + res_cog of group "GrpUnsorted" 8 0 @@ -347,7 +347,7 @@ - GrpUnsorted + group "GrpUnsorted" permute 2 1 8 2 diff --git a/src/gromacs/selection/tests/selectioncollection.cpp b/src/gromacs/selection/tests/selectioncollection.cpp index 8521a27d44..c04539a03f 100644 --- a/src/gromacs/selection/tests/selectioncollection.cpp +++ b/src/gromacs/selection/tests/selectioncollection.cpp @@ -972,7 +972,12 @@ TEST_F(SelectionCollectionDataTest, HandlesIndexGroupsInSelections) "group \"GrpA\"", "GrpB", "1", - "group \"GrpB\" and resname RB" + // These test that the name of the group is not too eagerly promoted to + // the name of the selection. + "group \"GrpB\" and resname RB", + "group \"GrpA\" permute 5 3 2 1 4", + "group \"GrpA\" plus group \"GrpB\"", + "res_cog of group \"GrpA\"" }; setFlags(TestFlags() | efTestSelectionNames); ASSERT_NO_THROW_GMX(loadIndexGroups("simple.ndx")); -- 2.22.0