Fix bug in selection subexpression handling.

Did not work correctly if
1. a static expression was passed to a SPAR_ATOMVAL parameter,
2. other parameters to the same selection method were dynamic
   (so that the expression would not be completely eliminated during
   compilation), and
3. the evaluation group of the SEL_EXPRESSION element was dynamic
   (so that the SPAR_ATOMVAL expression could be evaluated for a
   different group during evaluation and compilation).
In this case, the static atom-valued parameter got evaluated during
compilation for the maximal evaluation group and replaced by a constant.
During evaluation, if the evaluation group was smaller, the values for
that expression were no longer correct.
See #1216 for what kinds of concrete selections this applies to.

Subexpression handling in the selections is starting to (again) get very
messy, but that is a topic for a larger-scale refactoring...

Fixes #1216.

Change-Id: Ic6b7f9b8df661a9c78d7862b981a07e65a7ebdbf

diff --git a/src/gromacs/selection/compiler.cpp b/src/gromacs/selection/compiler.cpp
index 895a44b9c539e2dc04a23bd95b504c892a72dc81..76e083972ba1d2c88415df5542ebece2bcca548f 100644 (file)
--- a/src/gromacs/selection/compiler.cpp
+++ b/src/gromacs/selection/compiler.cpp
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2009,2010,2011,2012, by the GROMACS development team, led by
+ * Copyright (c) 2009,2010,2011,2012,2013, by the GROMACS development team, led by
  * David van der Spoel, Berk Hess, Erik Lindahl, and including many
  * others, as listed in the AUTHORS file in the top-level source
  * directory and at http://www.gromacs.org.
@@ -324,11 +324,18 @@ enum
     /** Whether memory has been allocated for \p gmin and \p gmax. */
     SEL_CDATA_MINMAXALLOC = 16,
     /** Whether to update \p gmin and \p gmax in static analysis. */
-    SEL_CDATA_DOMINMAX      = 128,
+    SEL_CDATA_DOMINMAX      = 256,
     /** Whether the subexpression uses simple pass evaluation functions. */
     SEL_CDATA_SIMPLESUBEXPR = 32,
+    /*! \brief
+     * Whether a static subexpression needs to support multiple evaluations.
+     *
+     * This flag may only be set on \ref SEL_SUBEXPR elements that also have
+     * SEL_CDATA_SIMPLESUBEXPR.
+     */
+    SEL_CDATA_STATICMULTIEVALSUBEXPR = 64,
     /** Whether this expression is a part of a common subexpression. */
-    SEL_CDATA_COMMONSUBEXPR = 64
+    SEL_CDATA_COMMONSUBEXPR = 128
 };
 
 /*! \internal \brief
@@ -418,6 +425,10 @@ _gmx_selelem_print_compiler_info(FILE *fp, const SelectionTreeElement &sel,
     {
         fprintf(fp, "Ss");
     }
+    if (sel.cdata->flags & SEL_CDATA_STATICMULTIEVALSUBEXPR)
+    {
+        fprintf(fp, "Sm");
+    }
     if (sel.cdata->flags & SEL_CDATA_COMMONSUBEXPR)
     {
         fprintf(fp, "Sc");
@@ -1139,9 +1150,15 @@ init_item_evalfunc(const SelectionTreeElementPointer &sel)
             break;
 
         case SEL_SUBEXPR:
-            sel->evaluate = ((sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR)
-                             ? &_gmx_sel_evaluate_subexpr_simple
-                             : &_gmx_sel_evaluate_subexpr);
+            if ((sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR)
+                && !(sel->cdata->flags & SEL_CDATA_STATICMULTIEVALSUBEXPR))
+            {
+                sel->evaluate = &_gmx_sel_evaluate_subexpr_simple;
+            }
+            else
+            {
+                sel->evaluate = &_gmx_sel_evaluate_subexpr;
+            }
             break;
 
         case SEL_SUBEXPRREF:
@@ -1217,7 +1234,8 @@ init_item_evaloutput(const SelectionTreeElementPointer &sel)
     }
 
     if (sel->type == SEL_SUBEXPR
-        && (sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR))
+        && (sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR)
+        && !(sel->cdata->flags & SEL_CDATA_STATICMULTIEVALSUBEXPR))
     {
         sel->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA);
         if (sel->v.type == GROUP_VALUE || sel->v.type == POS_VALUE)
@@ -1399,6 +1417,15 @@ init_item_staticeval(const SelectionTreeElementPointer &sel)
                     init_item_staticeval(child);
                 }
             }
+            /* If an expression is evaluated for a dynamic group, then also
+             * atom-valued parameters need to be evaluated every time. */
+            if ((sel->flags & SEL_DYNAMIC)
+                && (sel->type == SEL_EXPRESSION || sel->type == SEL_MODIFIER)
+                && (child->flags & SEL_ATOMVAL))
+            {
+                child->flags        |= SEL_DYNAMIC;
+                child->cdata->flags &= ~SEL_CDATA_STATIC;
+            }
             child = child->next;
         }
     }
@@ -1486,7 +1513,17 @@ init_item_subexpr_flags(const SelectionTreeElementPointer &sel)
     else if (sel->type == SEL_SUBEXPRREF
              && (sel->child->cdata->flags & SEL_CDATA_SIMPLESUBEXPR))
     {
-        sel->cdata->flags |= SEL_CDATA_SIMPLESUBEXPR;
+        /* See similar condition in init_item_staticeval(). */
+        if ((sel->flags & SEL_ATOMVAL)
+            && (sel->flags & SEL_DYNAMIC)
+            && !(sel->child->flags & SEL_DYNAMIC))
+        {
+            sel->child->cdata->flags |= SEL_CDATA_STATICMULTIEVALSUBEXPR;
+        }
+        else
+        {
+            sel->cdata->flags |= SEL_CDATA_SIMPLESUBEXPR;
+        }
     }
 
     /* Process children, but only follow subexpression references if the
@@ -1776,6 +1813,8 @@ store_param_val(const SelectionTreeElementPointer &sel)
     if (sel->v.type == INT_VALUE || sel->v.type == REAL_VALUE
         || sel->v.type == STR_VALUE)
     {
+        GMX_RELEASE_ASSERT(sel->v.u.ptr != NULL,
+                           "Selection method parameter not properly initialized");
         _gmx_selvalue_setstore(&sel->u.param->val, sel->v.u.ptr);
     }
 }
@@ -2196,7 +2235,9 @@ analyze_static(gmx_sel_evaluate_t                *data,
             break;
 
         case SEL_SUBEXPR:
-            if (sel->cdata->flags & (SEL_CDATA_SIMPLESUBEXPR | SEL_CDATA_FULLEVAL))
+            if (((sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR) &&
+                 !(sel->cdata->flags & SEL_CDATA_STATICMULTIEVALSUBEXPR))
+                || (sel->cdata->flags & SEL_CDATA_FULLEVAL))
             {
                 sel->cdata->evaluate(data, sel, g);
                 _gmx_selvalue_setstore(&sel->v, sel->child->v.u.ptr);
@@ -2496,6 +2537,17 @@ postprocess_item_subexpressions(const SelectionTreeElementPointer &sel)
         sel->child->flags   &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA);
         sel->child->v.nalloc = -1;
     }
+
+    /* For static subexpressions with a dynamic evaluation group, there is
+     * no need to evaluate them again, as the SEL_SUBEXPRREF takes care of
+     * everything during evaluation. */
+    if (sel->type == SEL_SUBEXPR
+        && (sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR)
+        && (sel->cdata->flags & SEL_CDATA_STATICMULTIEVALSUBEXPR))
+    {
+        sel->evaluate        = NULL;
+        sel->cdata->evaluate = NULL;
+    }
 }
 
 

diff --git a/src/gromacs/selection/evaluate.cpp b/src/gromacs/selection/evaluate.cpp
index f100465dc5fa6d5ad145fe3bdd4807d73c0b6644..cc1751b6c5b7f225daeacde77a8183092ea85408 100644 (file)
--- a/src/gromacs/selection/evaluate.cpp
+++ b/src/gromacs/selection/evaluate.cpp
@@ -405,14 +405,15 @@ SelectionEvaluator::evaluate(SelectionCollection *coll,
     while (sel)
     {
         /* Clear the evaluation group of subexpressions */
-        if (sel->child && sel->child->type == SEL_SUBEXPR)
+        if (sel->child && sel->child->type == SEL_SUBEXPR
+            && sel->child->evaluate != NULL)
         {
             sel->child->u.cgrp.isize = 0;
             /* Not strictly necessary, because the value will be overwritten
              * during first evaluation of the subexpression anyways, but we
              * clear the group for clarity. Note that this is _not_ done during
              * compilation because of some additional complexities involved
-             * (see compiler.c), so it should not be relied upon in
+             * (see compiler.cpp), so it should not be relied upon in
              * _gmx_sel_evaluate_subexpr(). */
             if (sel->child->v.type == GROUP_VALUE)
             {
@@ -778,7 +779,7 @@ _gmx_sel_evaluate_subexprref(gmx_sel_evaluate_t                *data,
 {
     int        i, j;
 
-    if (g)
+    if (g != NULL && sel->child->evaluate != NULL)
     {
         sel->child->evaluate(data, sel->child, g);
     }

diff --git a/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesDynamicAtomValuedParameters.xml b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesDynamicAtomValuedParameters.xml
new file mode 100644 (file)
index 0000000..bc1d89d
--- /dev/null
+++ b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesDynamicAtomValuedParameters.xml
@@ -0,0 +1,87 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/xsl" href="referencedata.xsl"?>
+<ReferenceData>
+  <ParsedSelections Name="Parsed">
+    <ParsedSelection Name="Selection1">
+      <String Name="Input">same residue as (atomnr 3 5 13 or y &gt; 5)</String>
+      <String Name="Name">same residue as (atomnr 3 5 13 or y &gt; 5)</String>
+      <String Name="Text">same residue as (atomnr 3 5 13 or y &gt; 5)</String>
+      <Bool Name="Dynamic">true</Bool>
+    </ParsedSelection>
+    <ParsedSelection Name="Selection2">
+      <String Name="Input">(resnr 1 3 5 or x &gt; 10) and same residue as (atomnr 3 5 13 or y &gt; 5)</String>
+      <String Name="Name">(resnr 1 3 5 or x &gt; 10) and same residue as (atomnr 3 5 13 or y &gt; 5)</String>
+      <String Name="Text">(resnr 1 3 5 or x &gt; 10) and same residue as (atomnr 3 5 13 or y &gt; 5)</String>
+      <Bool Name="Dynamic">true</Bool>
+    </ParsedSelection>
+  </ParsedSelections>
+  <CompiledSelections Name="Compiled">
+    <Selection Name="Selection1">
+      <Sequence Name="Atoms">
+        <Int Name="Length">15</Int>
+        <Int>0</Int>
+        <Int>1</Int>
+        <Int>2</Int>
+        <Int>3</Int>
+        <Int>4</Int>
+        <Int>5</Int>
+        <Int>6</Int>
+        <Int>7</Int>
+        <Int>8</Int>
+        <Int>9</Int>
+        <Int>10</Int>
+        <Int>11</Int>
+        <Int>12</Int>
+        <Int>13</Int>
+        <Int>14</Int>
+      </Sequence>
+    </Selection>
+    <Selection Name="Selection2">
+      <Sequence Name="Atoms">
+        <Int Name="Length">15</Int>
+        <Int>0</Int>
+        <Int>1</Int>
+        <Int>2</Int>
+        <Int>3</Int>
+        <Int>4</Int>
+        <Int>5</Int>
+        <Int>6</Int>
+        <Int>7</Int>
+        <Int>8</Int>
+        <Int>9</Int>
+        <Int>10</Int>
+        <Int>11</Int>
+        <Int>12</Int>
+        <Int>13</Int>
+        <Int>14</Int>
+      </Sequence>
+    </Selection>
+  </CompiledSelections>
+  <EvaluatedSelections Name="Frame1">
+    <Selection Name="Selection1">
+      <Sequence Name="Atoms">
+        <Int Name="Length">9</Int>
+        <Int>0</Int>
+        <Int>1</Int>
+        <Int>2</Int>
+        <Int>3</Int>
+        <Int>4</Int>
+        <Int>5</Int>
+        <Int>12</Int>
+        <Int>13</Int>
+        <Int>14</Int>
+      </Sequence>
+    </Selection>
+    <Selection Name="Selection2">
+      <Sequence Name="Atoms">
+        <Int Name="Length">6</Int>
+        <Int>0</Int>
+        <Int>1</Int>
+        <Int>2</Int>
+        <Int>12</Int>
+        <Int>13</Int>
+        <Int>14</Int>
+      </Sequence>
+    </Selection>
+  </EvaluatedSelections>
+</ReferenceData>

diff --git a/src/gromacs/selection/tests/selectioncollection.cpp b/src/gromacs/selection/tests/selectioncollection.cpp
index 95b6b507ef31f0b4f2fa05d62a32336d828e27e2..491cfbb22dbf18f6a228f238168e33640c4c135e 100644 (file)
--- a/src/gromacs/selection/tests/selectioncollection.cpp
+++ b/src/gromacs/selection/tests/selectioncollection.cpp
@@ -915,6 +915,17 @@ TEST_F(SelectionCollectionDataTest, HandlesBasicBoolean)
 }
 
 
+TEST_F(SelectionCollectionDataTest, HandlesDynamicAtomValuedParameters)
+{
+    static const char * const selections[] = {
+        "same residue as (atomnr 3 5 13 or y > 5)",
+        "(resnr 1 3 5 or x > 10) and same residue as (atomnr 3 5 13 or y > 5)"
+    };
+    setFlags(TestFlags() | efTestEvaluation);
+    runTest("simple.gro", selections);
+}
+
+
 TEST_F(SelectionCollectionDataTest, HandlesNumericComparisons)
 {
     static const char * const selections[] = {