Remove overeager selection index group check

The check for sorted groups in selections was a bit too eager, and
prevented use of many index groups that would previously have been
perfectly valid for, e.g., g_bond, with gmx distance.  Remove the
check.  Instead, fix the case where unsorted group is passed as the
value of a selection, and add tests for that.

Will reinstantiate the check later in another form, as currently
unsorted index groups get silently accepted and treated in a more or
less random manner in all other cases.  However, this change provides
a usability improvement that could be nice to have, even if I don't
immediately have time to implement the check.

Change-Id: I86e935d8bd08e63a9e3d01fb5a4084d96c5b7006

diff --git a/src/gromacs/selection/evaluate.cpp b/src/gromacs/selection/evaluate.cpp
index 0d67bb0c2d622f7bb622c530b916d6c4fcf72140..9b1fb6ed213baca33ef062148cdb4f38b2f14924 100644 (file)
--- a/src/gromacs/selection/evaluate.cpp
+++ b/src/gromacs/selection/evaluate.cpp
@@ -499,7 +499,16 @@ _gmx_sel_evaluate_static(gmx_sel_evaluate_t                      * /* data */,
                          const gmx::SelectionTreeElementPointer &sel,
                          gmx_ana_index_t                        *g)
 {
-    gmx_ana_index_intersection(sel->v.u.g, &sel->u.cgrp, g);
+    if (sel->flags & SEL_UNSORTED)
+    {
+        // This only works if g contains all the atoms, but that is currently
+        // the only supported case.
+        gmx_ana_index_copy(sel->v.u.g, &sel->u.cgrp, false);
+    }
+    else
+    {
+        gmx_ana_index_intersection(sel->v.u.g, &sel->u.cgrp, g);
+    }
 }
 
 

diff --git a/src/gromacs/selection/parsetree.cpp b/src/gromacs/selection/parsetree.cpp
index 7f4566923dc45d57f5ac53fdb56a28945cc19063..a40798df3505c731158290a505e76d9500c5ee64 100644 (file)
--- a/src/gromacs/selection/parsetree.cpp
+++ b/src/gromacs/selection/parsetree.cpp
@@ -410,8 +410,8 @@ _gmx_selelem_update_flags(const gmx::SelectionTreeElementPointer &sel,
     {
         /* Update the child */
         _gmx_selelem_update_flags(child, scanner);
-        /* Propagate the dynamic flag */
-        sel->flags |= (child->flags & SEL_DYNAMIC);
+        /* Propagate the dynamic and unsorted flags */
+        sel->flags |= (child->flags & (SEL_DYNAMIC | SEL_UNSORTED));
         /* Propagate the type flag if necessary and check for problems */
         if (bUseChildType)
         {

diff --git a/src/gromacs/selection/selectioncollection.cpp b/src/gromacs/selection/selectioncollection.cpp
index 09f24e03c5faf90968ef74b3c38fd6c1801f57a8..400e7d8d3b8f9444f5387e85347da19168202fe9 100644 (file)
--- a/src/gromacs/selection/selectioncollection.cpp
+++ b/src/gromacs/selection/selectioncollection.cpp
@@ -447,7 +447,8 @@ void SelectionCollection::Impl::resolveExternalGroups(
     while (child)
     {
         resolveExternalGroups(child, errors);
-        child = child->next;
+        root->flags |= (child->flags & SEL_UNSORTED);
+        child        = child->next;
     }
 }
 

diff --git a/src/gromacs/selection/selelem.cpp b/src/gromacs/selection/selelem.cpp
index 01d7c6f77a6adf7bccec8536245294753a908358..e80e4f75c8d241d946505ec85d6e5901a2666206 100644 (file)
--- a/src/gromacs/selection/selelem.cpp
+++ b/src/gromacs/selection/selelem.cpp
@@ -365,6 +365,9 @@ void SelectionTreeElement::resolveIndexGroupReference(gmx_ana_indexgrps_t *grps)
 
     if (!gmx_ana_index_check_sorted(&foundGroup))
     {
+        flags |= SEL_UNSORTED;
+        // TODO: Add this test elsewhere, where it does not break valid use cases.
+#if 0
         gmx_ana_index_deinit(&foundGroup);
         std::string message = formatString(
                     "Group '%s' ('%s') cannot be used in selections, "
@@ -372,6 +375,7 @@ void SelectionTreeElement::resolveIndexGroupReference(gmx_ana_indexgrps_t *grps)
                     "it contains duplicate atoms.",
                     foundName.c_str(), name().c_str());
         GMX_THROW(InconsistentInputError(message));
+#endif
     }
 
     sfree(u.gref.name);

diff --git a/src/gromacs/selection/selelem.h b/src/gromacs/selection/selelem.h
index d5f44a9156ffcb9e01dc31e549ae9734d30ff38e..e47d162c0cd10f885408e3ac34cf37a889246158 100644 (file)
--- a/src/gromacs/selection/selelem.h
+++ b/src/gromacs/selection/selelem.h
@@ -167,6 +167,10 @@ _gmx_sel_value_type_str(const gmx_ana_selvalue_t *val);
  * The element (or one of its children) is dynamic.
  */
 #define SEL_DYNAMIC     16
+/*! \brief
+ * The element may contain atom indices in an unsorted order.
+ */
+#define SEL_UNSORTED    32
 /*! \brief
  * Mask that covers the flags that describe the number of values.
  */

diff --git a/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesUnsortedIndexGroupsInSelections.xml b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesUnsortedIndexGroupsInSelections.xml
new file mode 100644 (file)
index 0000000..ddcda26
--- /dev/null
+++ b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesUnsortedIndexGroupsInSelections.xml
@@ -0,0 +1,422 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/xsl" href="referencedata.xsl"?>
+<ReferenceData>
+  <ParsedSelections Name="Parsed">
+    <ParsedSelection Name="Selection1">
+      <String Name="Input">group "GrpUnsorted"</String>
+      <String Name="Name">GrpUnsorted</String>
+      <String Name="Text">group "GrpUnsorted"</String>
+      <Bool Name="Dynamic">false</Bool>
+    </ParsedSelection>
+    <ParsedSelection Name="Selection2">
+      <String Name="Input">GrpUnsorted</String>
+      <String Name="Name">GrpUnsorted</String>
+      <String Name="Text">GrpUnsorted</String>
+      <Bool Name="Dynamic">false</Bool>
+    </ParsedSelection>
+    <ParsedSelection Name="Selection3">
+      <String Name="Input">2</String>
+      <String Name="Name">GrpUnsorted</String>
+      <String Name="Text">2</String>
+      <Bool Name="Dynamic">false</Bool>
+    </ParsedSelection>
+    <ParsedSelection Name="Selection4">
+      <String Name="Input">res_cog of group "GrpUnsorted"</String>
+      <String Name="Name">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="Text">group "GrpUnsorted" permute 2 1</String>
+      <Bool Name="Dynamic">false</Bool>
+    </ParsedSelection>
+  </ParsedSelections>
+  <CompiledSelections Name="Compiled">
+    <Selection Name="Selection1">
+      <String Name="Name">GrpUnsorted</String>
+      <Sequence Name="Atoms">
+        <Int Name="Length">8</Int>
+        <Int>0</Int>
+        <Int>2</Int>
+        <Int>1</Int>
+        <Int>8</Int>
+        <Int>6</Int>
+        <Int>4</Int>
+        <Int>2</Int>
+        <Int>11</Int>
+      </Sequence>
+      <Sequence Name="Positions">
+        <Int Name="Length">8</Int>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>0</Int>
+          </Sequence>
+          <Int Name="RefId">0</Int>
+          <Int Name="MappedId">0</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>2</Int>
+          </Sequence>
+          <Int Name="RefId">1</Int>
+          <Int Name="MappedId">2</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>1</Int>
+          </Sequence>
+          <Int Name="RefId">2</Int>
+          <Int Name="MappedId">1</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>8</Int>
+          </Sequence>
+          <Int Name="RefId">3</Int>
+          <Int Name="MappedId">8</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>6</Int>
+          </Sequence>
+          <Int Name="RefId">4</Int>
+          <Int Name="MappedId">6</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>4</Int>
+          </Sequence>
+          <Int Name="RefId">5</Int>
+          <Int Name="MappedId">4</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>2</Int>
+          </Sequence>
+          <Int Name="RefId">6</Int>
+          <Int Name="MappedId">2</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>11</Int>
+          </Sequence>
+          <Int Name="RefId">7</Int>
+          <Int Name="MappedId">11</Int>
+        </Position>
+      </Sequence>
+    </Selection>
+    <Selection Name="Selection2">
+      <String Name="Name">GrpUnsorted</String>
+      <Sequence Name="Atoms">
+        <Int Name="Length">8</Int>
+        <Int>0</Int>
+        <Int>2</Int>
+        <Int>1</Int>
+        <Int>8</Int>
+        <Int>6</Int>
+        <Int>4</Int>
+        <Int>2</Int>
+        <Int>11</Int>
+      </Sequence>
+      <Sequence Name="Positions">
+        <Int Name="Length">8</Int>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>0</Int>
+          </Sequence>
+          <Int Name="RefId">0</Int>
+          <Int Name="MappedId">0</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>2</Int>
+          </Sequence>
+          <Int Name="RefId">1</Int>
+          <Int Name="MappedId">2</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>1</Int>
+          </Sequence>
+          <Int Name="RefId">2</Int>
+          <Int Name="MappedId">1</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>8</Int>
+          </Sequence>
+          <Int Name="RefId">3</Int>
+          <Int Name="MappedId">8</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>6</Int>
+          </Sequence>
+          <Int Name="RefId">4</Int>
+          <Int Name="MappedId">6</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>4</Int>
+          </Sequence>
+          <Int Name="RefId">5</Int>
+          <Int Name="MappedId">4</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>2</Int>
+          </Sequence>
+          <Int Name="RefId">6</Int>
+          <Int Name="MappedId">2</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>11</Int>
+          </Sequence>
+          <Int Name="RefId">7</Int>
+          <Int Name="MappedId">11</Int>
+        </Position>
+      </Sequence>
+    </Selection>
+    <Selection Name="Selection3">
+      <String Name="Name">GrpUnsorted</String>
+      <Sequence Name="Atoms">
+        <Int Name="Length">8</Int>
+        <Int>0</Int>
+        <Int>2</Int>
+        <Int>1</Int>
+        <Int>8</Int>
+        <Int>6</Int>
+        <Int>4</Int>
+        <Int>2</Int>
+        <Int>11</Int>
+      </Sequence>
+      <Sequence Name="Positions">
+        <Int Name="Length">8</Int>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>0</Int>
+          </Sequence>
+          <Int Name="RefId">0</Int>
+          <Int Name="MappedId">0</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>2</Int>
+          </Sequence>
+          <Int Name="RefId">1</Int>
+          <Int Name="MappedId">2</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>1</Int>
+          </Sequence>
+          <Int Name="RefId">2</Int>
+          <Int Name="MappedId">1</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>8</Int>
+          </Sequence>
+          <Int Name="RefId">3</Int>
+          <Int Name="MappedId">8</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>6</Int>
+          </Sequence>
+          <Int Name="RefId">4</Int>
+          <Int Name="MappedId">6</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>4</Int>
+          </Sequence>
+          <Int Name="RefId">5</Int>
+          <Int Name="MappedId">4</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>2</Int>
+          </Sequence>
+          <Int Name="RefId">6</Int>
+          <Int Name="MappedId">2</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>11</Int>
+          </Sequence>
+          <Int Name="RefId">7</Int>
+          <Int Name="MappedId">11</Int>
+        </Position>
+      </Sequence>
+    </Selection>
+    <Selection Name="Selection4">
+      <String Name="Name">GrpUnsorted</String>
+      <Sequence Name="Atoms">
+        <Int Name="Length">8</Int>
+        <Int>0</Int>
+        <Int>2</Int>
+        <Int>1</Int>
+        <Int>8</Int>
+        <Int>6</Int>
+        <Int>4</Int>
+        <Int>2</Int>
+        <Int>11</Int>
+      </Sequence>
+      <Sequence Name="Positions">
+        <Int Name="Length">5</Int>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">3</Int>
+            <Int>0</Int>
+            <Int>2</Int>
+            <Int>1</Int>
+          </Sequence>
+          <Int Name="RefId">0</Int>
+          <Int Name="MappedId">0</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">2</Int>
+            <Int>8</Int>
+            <Int>6</Int>
+          </Sequence>
+          <Int Name="RefId">1</Int>
+          <Int Name="MappedId">2</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>4</Int>
+          </Sequence>
+          <Int Name="RefId">2</Int>
+          <Int Name="MappedId">1</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>2</Int>
+          </Sequence>
+          <Int Name="RefId">3</Int>
+          <Int Name="MappedId">0</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>11</Int>
+          </Sequence>
+          <Int Name="RefId">4</Int>
+          <Int Name="MappedId">3</Int>
+        </Position>
+      </Sequence>
+    </Selection>
+    <Selection Name="Selection5">
+      <String Name="Name">GrpUnsorted</String>
+      <Sequence Name="Atoms">
+        <Int Name="Length">8</Int>
+        <Int>2</Int>
+        <Int>0</Int>
+        <Int>8</Int>
+        <Int>1</Int>
+        <Int>4</Int>
+        <Int>6</Int>
+        <Int>11</Int>
+        <Int>2</Int>
+      </Sequence>
+      <Sequence Name="Positions">
+        <Int Name="Length">8</Int>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>2</Int>
+          </Sequence>
+          <Int Name="RefId">0</Int>
+          <Int Name="MappedId">2</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>0</Int>
+          </Sequence>
+          <Int Name="RefId">1</Int>
+          <Int Name="MappedId">0</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>8</Int>
+          </Sequence>
+          <Int Name="RefId">2</Int>
+          <Int Name="MappedId">8</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>1</Int>
+          </Sequence>
+          <Int Name="RefId">3</Int>
+          <Int Name="MappedId">1</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>4</Int>
+          </Sequence>
+          <Int Name="RefId">4</Int>
+          <Int Name="MappedId">4</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>6</Int>
+          </Sequence>
+          <Int Name="RefId">5</Int>
+          <Int Name="MappedId">6</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>11</Int>
+          </Sequence>
+          <Int Name="RefId">6</Int>
+          <Int Name="MappedId">11</Int>
+        </Position>
+        <Position>
+          <Sequence Name="Atoms">
+            <Int Name="Length">1</Int>
+            <Int>2</Int>
+          </Sequence>
+          <Int Name="RefId">7</Int>
+          <Int Name="MappedId">2</Int>
+        </Position>
+      </Sequence>
+    </Selection>
+  </CompiledSelections>
+</ReferenceData>

diff --git a/src/gromacs/selection/tests/selectioncollection.cpp b/src/gromacs/selection/tests/selectioncollection.cpp
index f106e2f4afd5c0fea9fb5ffcc2a8a4f0a50b66e9..0727c881f00eba89fee14d79c396cf72c592cadf 100644 (file)
--- a/src/gromacs/selection/tests/selectioncollection.cpp
+++ b/src/gromacs/selection/tests/selectioncollection.cpp
@@ -487,7 +487,9 @@ TEST_F(SelectionCollectionTest, HandlesUnknownGroupReferenceDelayed2)
     EXPECT_THROW_GMX(sc_.compile(), gmx::APIError);
 }
 
-TEST_F(SelectionCollectionTest, HandlesUnsortedGroupReference)
+// TODO: Make the check less eager so that it doesn't break other tests, and
+// adapt these tests accordingly.
+TEST_F(SelectionCollectionTest, DISABLED_HandlesUnsortedGroupReference)
 {
     ASSERT_NO_THROW_GMX(loadIndexGroups("simple.ndx"));
     EXPECT_THROW_GMX(sc_.parseFromString("group \"GrpUnsorted\""),
@@ -495,7 +497,7 @@ TEST_F(SelectionCollectionTest, HandlesUnsortedGroupReference)
     EXPECT_THROW_GMX(sc_.parseFromString("2"), gmx::InconsistentInputError);
 }
 
-TEST_F(SelectionCollectionTest, HandlesUnsortedGroupReferenceDelayed)
+TEST_F(SelectionCollectionTest, DISABLED_HandlesUnsortedGroupReferenceDelayed)
 {
     ASSERT_NO_THROW_GMX(sc_.parseFromString("group 2; group \"GrpUnsorted\""));
     EXPECT_THROW_GMX(loadIndexGroups("simple.ndx"), gmx::InconsistentInputError);
@@ -924,6 +926,21 @@ TEST_F(SelectionCollectionDataTest, HandlesIndexGroupsInSelectionsDelayed)
     ASSERT_NO_FATAL_FAILURE(runCompiler());
 }
 
+TEST_F(SelectionCollectionDataTest, HandlesUnsortedIndexGroupsInSelections)
+{
+    static const char * const selections[] = {
+        "group \"GrpUnsorted\"",
+        "GrpUnsorted",
+        "2",
+        "res_cog of group \"GrpUnsorted\"",
+        "group \"GrpUnsorted\" permute 2 1"
+    };
+    setFlags(TestFlags() | efTestPositionAtoms | efTestPositionMapping
+             | efTestSelectionNames);
+    ASSERT_NO_THROW_GMX(loadIndexGroups("simple.ndx"));
+    runTest("simple.gro", selections);
+}
+
 TEST_F(SelectionCollectionDataTest, HandlesConstantPositions)
 {
     static const char * const selections[] = {

diff --git a/src/gromacs/selection/tests/simple.ndx b/src/gromacs/selection/tests/simple.ndx
index 4ca787321aa04c9f736aa7e0f8bd60c612d94818..99aceb54395f3e19e0449c866c030f5f9ed9f121 100644 (file)
--- a/src/gromacs/selection/tests/simple.ndx
+++ b/src/gromacs/selection/tests/simple.ndx
@@ -5,4 +5,4 @@
 4 5 6 7 8
 
 [ GrpUnsorted ]
-1 2 4 3 2
+1 3 2 9 7 5 3 12