From: Teemu Murtola Date: Sat, 28 Jun 2014 06:51:00 +0000 (+0300) Subject: Remove overeager selection index group check X-Git-Url: http://biod.pnpi.spb.ru/gitweb/?a=commitdiff_plain;h=7a79695704adc869e45799404ef352fd7f00bf3d;p=alexxy%2Fgromacs.git 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 0d67bb0c2d..9b1fb6ed21 100644 --- 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 7f4566923d..a40798df35 100644 --- 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 09f24e03c5..400e7d8d3b 100644 --- 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 01d7c6f77a..e80e4f75c8 100644 --- 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 d5f44a9156..e47d162c0c 100644 --- 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 index 0000000000..ddcda26bc4 --- /dev/null +++ b/src/gromacs/selection/tests/refdata/SelectionCollectionDataTest_HandlesUnsortedIndexGroupsInSelections.xml @@ -0,0 +1,422 @@ + + + + + + group "GrpUnsorted" + GrpUnsorted + group "GrpUnsorted" + false + + + GrpUnsorted + GrpUnsorted + GrpUnsorted + false + + + 2 + GrpUnsorted + 2 + false + + + res_cog of group "GrpUnsorted" + GrpUnsorted + res_cog of group "GrpUnsorted" + false + + + group "GrpUnsorted" permute 2 1 + GrpUnsorted + group "GrpUnsorted" permute 2 1 + false + + + + + GrpUnsorted + + 8 + 0 + 2 + 1 + 8 + 6 + 4 + 2 + 11 + + + 8 + + + 1 + 0 + + 0 + 0 + + + + 1 + 2 + + 1 + 2 + + + + 1 + 1 + + 2 + 1 + + + + 1 + 8 + + 3 + 8 + + + + 1 + 6 + + 4 + 6 + + + + 1 + 4 + + 5 + 4 + + + + 1 + 2 + + 6 + 2 + + + + 1 + 11 + + 7 + 11 + + + + + GrpUnsorted + + 8 + 0 + 2 + 1 + 8 + 6 + 4 + 2 + 11 + + + 8 + + + 1 + 0 + + 0 + 0 + + + + 1 + 2 + + 1 + 2 + + + + 1 + 1 + + 2 + 1 + + + + 1 + 8 + + 3 + 8 + + + + 1 + 6 + + 4 + 6 + + + + 1 + 4 + + 5 + 4 + + + + 1 + 2 + + 6 + 2 + + + + 1 + 11 + + 7 + 11 + + + + + GrpUnsorted + + 8 + 0 + 2 + 1 + 8 + 6 + 4 + 2 + 11 + + + 8 + + + 1 + 0 + + 0 + 0 + + + + 1 + 2 + + 1 + 2 + + + + 1 + 1 + + 2 + 1 + + + + 1 + 8 + + 3 + 8 + + + + 1 + 6 + + 4 + 6 + + + + 1 + 4 + + 5 + 4 + + + + 1 + 2 + + 6 + 2 + + + + 1 + 11 + + 7 + 11 + + + + + GrpUnsorted + + 8 + 0 + 2 + 1 + 8 + 6 + 4 + 2 + 11 + + + 5 + + + 3 + 0 + 2 + 1 + + 0 + 0 + + + + 2 + 8 + 6 + + 1 + 2 + + + + 1 + 4 + + 2 + 1 + + + + 1 + 2 + + 3 + 0 + + + + 1 + 11 + + 4 + 3 + + + + + GrpUnsorted + + 8 + 2 + 0 + 8 + 1 + 4 + 6 + 11 + 2 + + + 8 + + + 1 + 2 + + 0 + 2 + + + + 1 + 0 + + 1 + 0 + + + + 1 + 8 + + 2 + 8 + + + + 1 + 1 + + 3 + 1 + + + + 1 + 4 + + 4 + 4 + + + + 1 + 6 + + 5 + 6 + + + + 1 + 11 + + 6 + 11 + + + + 1 + 2 + + 7 + 2 + + + + + diff --git a/src/gromacs/selection/tests/selectioncollection.cpp b/src/gromacs/selection/tests/selectioncollection.cpp index f106e2f4af..0727c881f0 100644 --- 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 4ca787321a..99aceb5439 100644 --- 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