Project

General

Profile

Bug #1216

Selections that contain "same ... as" in a complex boolean expression are incorrectly evaluated

Added by Teemu Murtola over 4 years ago. Updated about 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
selections
Target version:
Affected version - extra info:
4.5-4.5.6, 4.6-4.6.1
Affected version:
Difficulty:
uncategorized
Close

Description

Example selection from the gmx-users mailing list:

shell = group "SOL" and same residue as within 0.4 of group "protein";
shell1 = group "SOL" and same residue as not within 0.3 of group "protein";
shell2 = shell and shell1;
shell2;

Permuting the shell and shell1 variables in the definition of shell2, or adding extra selections that evaluate shell and shell1 reveal different kinds of symptoms, ranging from incorrect results to internal errors.

Removing the same residue as part from the selections makes them work.


Related issues

Related to GROMACS - Bug #1219: Selections that use a dynamically evaluated variable in different context may be evaluated incorrectly Closed 04/10/2013

Associated revisions

Revision 076c4dec (diff)
Added by Teemu Murtola over 4 years ago

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

Revision a301d73c (diff)
Added by Teemu Murtola over 4 years ago

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.

Fixes #1216.

Backported from master with the same Change-Id.
Needed to also pull in part of the changes from
6e877567d9d3d08a3f6fb436f7bbfbaf35b95f8e.

Change-Id: Ic6b7f9b8df661a9c78d7862b981a07e65a7ebdbf

Revision fc80076f (diff)
Added by Teemu Murtola about 4 years ago

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

History

#1 Updated by Teemu Murtola over 4 years ago

I have identified at least one problem, which I need to fix to see whether there are more.

This is a general issue, but only occurs with the same ... as construct in most scenarios. The issue occurs with any selection of that contains an expression like this:

EXPR1 and same ANY as EXPR2

where ANY can be any keyword acceptable in that context, and EXPR1 and EXPR2 are both dynamic selections, i.e., depend on the trajectory frame. If there is only one same keyword in the selection, then it is possible to write it as (same ANY as EXPR2) and EXPR2, and it should work.

The issue should also occur if the selection contains

EXPR1 and EXPR2 < EXPR3

where EXPR1 is dynamic, and exactly one of EXPR2 and EXPR3 is dynamic. Selections like x < occupancy are an example of this, but one would rarely want to do something like this.

#2 Updated by Teemu Murtola over 4 years ago

  • Target version set to 4.5.7
  • Affected version - extra info set to 4.5-4.5.6, 4.6-4.6.1

Fix and a backport of it are now in gerrit. The related bug is about the internal error, which turned out to be a separate problem.

#3 Updated by Teemu Murtola over 4 years ago

  • Status changed from New to In Progress

#4 Updated by Mark Abraham over 4 years ago

  • Status changed from In Progress to Resolved
  • Affected version set to 4.5.6

#5 Updated by Teemu Murtola over 4 years ago

This is not yet fully resolved, since https://gerrit.gromacs.org/#/c/2315/ (which fixes this for master, and also adds unit tests) is not yet merged. However, it is not possible to change the status back...

#6 Updated by Mark Abraham over 4 years ago

  • Status changed from Resolved to Feedback wanted

#7 Updated by Mark Abraham over 4 years ago

  • Status changed from Feedback wanted to In Progress

#8 Updated by Teemu Murtola over 4 years ago

  • Status changed from In Progress to Fix uploaded

#9 Updated by Mark Abraham over 4 years ago

  • Status changed from Fix uploaded to Resolved

#10 Updated by Teemu Murtola over 4 years ago

Nothing has changed after I wrote comment 5, so Resolved is still not the best possible state. But do as you wish, I'm starting to get tired... Plus, it seems that Resolved makes the issue disappear from standard queries, which doesn't seem appropriate...

#11 Updated by Mark Abraham over 4 years ago

  • Status changed from Resolved to Feedback wanted
  • Target version changed from 4.5.7 to 5.0

Teemu Murtola wrote:

Nothing has changed after I wrote comment 5, so Resolved is still not the best possible state.

Sorry, I missed that you referred to master branch in #5 while "batch" processing a hundred commits with targets that had already been released. When I last re-set the status here I thought the context was for 4.5/4.6.

But do as you wish, I'm starting to get tired...

Sorry, I'm human and would need several clones to give everything the attention I would like to. :-)

Plus, it seems that Resolved makes the issue disappear from standard queries, which doesn't seem appropriate...

I tweaked today whether Resolved made an issue open or closed, while trying to see how best to clear up the roadmap. It now seems unnecessary - not sure whether closing the old targets or adjusting the targets was effective there. So I have returned Resolved to "issue is not closed". This is distinct from Closed status. I too am sick of managing software when I don't have time to read what software documentation exists, nor document how we should use our implementation of it. :-)

#12 Updated by Mark Abraham over 4 years ago

  • Status changed from Feedback wanted to In Progress

#13 Updated by Teemu Murtola over 4 years ago

  • Status changed from In Progress to Resolved

Now the master change has been merged, closing.

This issue was a bit of a special case, since I implemented it first in master and then backported the issue to the 4.5 branch. I wanted to do that at the same time rather than wait for the possibly lengthy review of the master commit first; otherwise, it would have never made it to 4.5.7 (although I think I uploaded the backport before 4.5.7 schedule was announced).

I am willing to at least write some of that documentation for using Redmine, but my previous experiences are not encouraging for actually spending a lot of time in writing thought-out documentation. It is more a rule than an exception that when I've been prodded enough to write something to the wiki, I hardly ever get any feedback on whether that was what was wanted, whether it is useful, etc. Not from people who requested the documentation, or from anyone else. To avoid spending my free time on issues that don't seem to interest anyone, I have switched my mode of operation to only write rough drafts (mainly in Redmine issues) and wait to get at least some feedback before spending any more time. Unfortunately, that has usually meant that those rough drafts are the only things that ever get done...

#14 Updated by Teemu Murtola over 4 years ago

  • Status changed from Resolved to Closed

#15 Updated by Mark Abraham over 4 years ago

Teemu Murtola wrote:

Now the master change has been merged, closing.

This issue was a bit of a special case, since I implemented it first in master and then backported the issue to the 4.5 branch. I wanted to do that at the same time rather than wait for the possibly lengthy review of the master commit first; otherwise, it would have never made it to 4.5.7 (although I think I uploaded the backport before 4.5.7 schedule was announced).

Yes. Thank you for your diligence.

I am willing to at least write some of that documentation for using Redmine, but my previous experiences are not encouraging for actually spending a lot of time in writing thought-out documentation. It is more a rule than an exception that when I've been prodded enough to write something to the wiki, I hardly ever get any feedback on whether that was what was wanted, whether it is useful, etc. Not from people who requested the documentation, or from anyone else. To avoid spending my free time on issues that don't seem to interest anyone, I have switched my mode of operation to only write rough drafts (mainly in Redmine issues) and wait to get at least some feedback before spending any more time. Unfortunately, that has usually meant that those rough drafts are the only things that ever get done...

Yes. We remain extremely grateful for the contribution you have made over such a long time. The nature of a project that proceeds by consensus of discussion over written fora when (mostly) volunteers have time is to be slow and sporadic. The teleconferences fell into silence because I ran out of ideas good for round table discussion. Is there material you would like to put forward for such discussion (even in your absence?)

The GROMACS project will hopefully be getting some more love from Sander and Rossen in the coming months. 4.6 activity is winding down also. So I am hopeful that discussion lag times can improve.

#16 Updated by Teemu Murtola about 4 years ago

  • Category changed from analysis tools to selections

Also available in: Atom PDF