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

diff --git a/src/gromacs/selection/keywords.h b/src/gromacs/selection/keywords.h
index eb503a7e7e9d4350fc817b200e758a9445edd8c8..6726dbb03ba483a9f444ac5231ba590a14da3eed 100644 (file)
--- 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 8cca62b067b9f7143ff6a216e04f7ccda8a4e80b..64013091dcc152a799bec066a97f587c6d170c29 100644 (file)
--- 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 fe85404536f1c6e8bf3355c3cbd7f7da6c270ec4..ad174aa1591118fbec7f170d7de7348663886637 100644 (file)
--- 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<t_methoddata_pos *>(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 9cd47b98bb2bab565b693486a3bb8f6b8d625a37..8cae2e62f03827d43eeb3af8ab496d14800dd38a 100644 (file)
--- a/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesIndexGroupsInSelections.xml
+++ b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesIndexGroupsInSelections.xml
@@ -26,6 +26,24 @@
       <String Name="Text">group "GrpB" and resname RB</String>
       <Bool Name="Dynamic">false</Bool>
     </ParsedSelection>
+    <ParsedSelection Name="Selection5">
+      <String Name="Input">group "GrpA" permute 5 3 2 1 4</String>
+      <String Name="Name">group "GrpA" permute 5 3 2 1 4</String>
+      <String Name="Text">group "GrpA" permute 5 3 2 1 4</String>
+      <Bool Name="Dynamic">false</Bool>
+    </ParsedSelection>
+    <ParsedSelection Name="Selection6">
+      <String Name="Input">group "GrpA" plus group "GrpB"</String>
+      <String Name="Name">group "GrpA" plus group "GrpB"</String>
+      <String Name="Text">group "GrpA" plus group "GrpB"</String>
+      <Bool Name="Dynamic">false</Bool>
+    </ParsedSelection>
+    <ParsedSelection Name="Selection7">
+      <String Name="Input">res_cog of group "GrpA"</String>
+      <String Name="Name">res_cog of group "GrpA"</String>
+      <String Name="Text">res_cog of group "GrpA"</String>
+      <Bool Name="Dynamic">false</Bool>
+    </ParsedSelection>
   </ParsedSelections>
   <CompiledSelections Name="Compiled">
     <Selection Name="Selection1">
@@ -70,5 +88,43 @@
         <Int>5</Int>
       </Sequence>
     </Selection>
+    <Selection Name="Selection5">
+      <String Name="Name">group "GrpA" permute 5 3 2 1 4</String>
+      <Sequence Name="Atoms">
+        <Int Name="Length">5</Int>
+        <Int>3</Int>
+        <Int>2</Int>
+        <Int>1</Int>
+        <Int>4</Int>
+        <Int>0</Int>
+      </Sequence>
+    </Selection>
+    <Selection Name="Selection6">
+      <String Name="Name">group "GrpA" plus group "GrpB"</String>
+      <Sequence Name="Atoms">
+        <Int Name="Length">10</Int>
+        <Int>0</Int>
+        <Int>1</Int>
+        <Int>2</Int>
+        <Int>3</Int>
+        <Int>4</Int>
+        <Int>3</Int>
+        <Int>4</Int>
+        <Int>5</Int>
+        <Int>6</Int>
+        <Int>7</Int>
+      </Sequence>
+    </Selection>
+    <Selection Name="Selection7">
+      <String Name="Name">res_cog of group "GrpA"</String>
+      <Sequence Name="Atoms">
+        <Int Name="Length">5</Int>
+        <Int>0</Int>
+        <Int>1</Int>
+        <Int>2</Int>
+        <Int>3</Int>
+        <Int>4</Int>
+      </Sequence>
+    </Selection>
   </CompiledSelections>
 </ReferenceData>

diff --git a/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesUnsortedIndexGroupsInSelections.xml b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesUnsortedIndexGroupsInSelections.xml
index cc54442ee6b41ebd6748db90dd2807739ab77c16..67a286d68099fbc617eba7928d705fe9972347ca 100644 (file)
--- a/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesUnsortedIndexGroupsInSelections.xml
+++ b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesUnsortedIndexGroupsInSelections.xml
@@ -25,13 +25,13 @@
     </ParsedSelection>
     <ParsedSelection Name="Selection4">
       <String Name="Input">res_cog of group "GrpUnsorted"</String>
-      <String Name="Name">GrpUnsorted</String>
+      <String Name="Name">res_cog of group "GrpUnsorted"</String>
       <String Name="Text">res_cog of group "GrpUnsorted"</String>
       <Bool Name="Dynamic">false</Bool>
     </ParsedSelection>
     <ParsedSelection Name="Selection5">
       <String Name="Input">group "GrpUnsorted" permute 2 1</String>
-      <String Name="Name">GrpUnsorted</String>
+      <String Name="Name">group "GrpUnsorted" permute 2 1</String>
       <String Name="Text">group "GrpUnsorted" permute 2 1</String>
       <Bool Name="Dynamic">false</Bool>
     </ParsedSelection>
@@ -287,7 +287,7 @@
       </Sequence>
     </Selection>
     <Selection Name="Selection4">
-      <String Name="Name">GrpUnsorted</String>
+      <String Name="Name">res_cog of group "GrpUnsorted"</String>
       <Sequence Name="Atoms">
         <Int Name="Length">8</Int>
         <Int>0</Int>
@@ -347,7 +347,7 @@
       </Sequence>
     </Selection>
     <Selection Name="Selection5">
-      <String Name="Name">GrpUnsorted</String>
+      <String Name="Name">group "GrpUnsorted" permute 2 1</String>
       <Sequence Name="Atoms">
         <Int Name="Length">8</Int>
         <Int>2</Int>

diff --git a/src/gromacs/selection/tests/selectioncollection.cpp b/src/gromacs/selection/tests/selectioncollection.cpp
index 8521a27d444767bb0e6bedd6768cb07f3564174e..c04539a03fe54ac7c4631ef734fe1af79824c7e9 100644 (file)
--- 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"));