Project

General

Profile

Bug #2847

Index groups (-n) in selections do not work without -s

Added by Teemu Murtola 3 months ago. Updated 2 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
analysis tools
Target version:
Affected version - extra info:
Affected version:
Difficulty:
uncategorized
Close

Description

If you use a tool that understands selections, but does not require the -s flag for basic use, it is impossible to provide the selection through an index file (-n) if -s is not provided. This gives an error that the maximum allowed atom index is zero. One example of an affected tool is gmx rdf. This works in release-2018, but not in release-2019 or master.

This seems to have got broken in I2f43e62bc2d97f5e654f15c6e474b9b71d7106ec, which changed the semantics of TopologyInformation. All comments in the class still say that it returns nullptr as the topology when no topology is loaded, and the selection code relies on that, but that no longer matches the implementation. Originally the class was a simple proxy to accessing different topology-related information in the tools, but it got a lot of other responsibilities as well with the mentioned change. The desired behavior of the class should be decided on (and/or the different responsibilities split), the documentation fixed to match the behavior, and all uses gone through to check that they actually use it as the decided behavior requires. It is likely that currently different parts of the code rely on conflicting behavior... Not sure if there are also other corner cases that may be broken.


Related issues

Related to GROMACS - Task #1862: Fully replace t_topology by gmx_mtop_tNew

Associated revisions

Revision 70d0bfa5 (diff)
Added by Mark Abraham 2 months ago

Revert changes to implementation of insert-molecules and solvate

This is a partial reversion of 5181f8d1, which made an inappropriate
dependency from preparation tools to TopologyInformation, which is
intended for analysis tools. This is needed in order to fix some
actual analysis tools that were broken by related changes to
TopologyInformation.

Unfortunately in 5181f8d1 readConformation was replaced with
TopologyInformation, and the former is no longer available. Instead,
readConfAndTopology is used in the same way that it was originally
used to implement readConformation. So the behaviour is reverted
even though the code looks different.

Refs #2847, #1862

Change-Id: I6e4900eea38b5c2e1b85b0d48427dabe26b09f9d

Revision 41347939 (diff)
Added by Mark Abraham 2 months ago

Fix use of selections referring to index groups

Selections that refer only to groups in an index file do not require a
topology file. This (and other behaviours) was inadvertently broken
by the wrong implementation of the default constructor, because that
constructor is used in setting up the analysis environment.

The default constructor of TopologyInformation should establish the
intended invariant, namely that until a topology is loaded, topology
getters should return nullptr.

Fixed that constructor, and fixed the unit test that verifies that
behaviour. Since that test was added at the same time as a change to
the implementation of TopologyInformation in commit 264458c9, the
broken invariant wasn't noticed.

Added a test also to gmx rdf that verifies that the selection
behaviour is fixed.

Fixes #2847

Change-Id: I2149616e0248c6d5cdd5657a8292d2c3346614b9

History

#1 Updated by Teemu Murtola 3 months ago

  • Description updated (diff)

Clarified the description.

#2 Updated by Gerrit Code Review Bot 3 months ago

Gerrit received a related patchset '1' for Issue #2847.
Uploader: Mark Abraham ()
Change-Id: gromacs~release-2019~I2149616e0248c6d5cdd5657a8292d2c3346614b9
Gerrit URL: https://gerrit.gromacs.org/9055

#3 Updated by Gerrit Code Review Bot 3 months ago

Gerrit received a related DRAFT patchset '1' for Issue #2847.
Uploader: Mark Abraham ()
Change-Id: gromacs~master~I153f0530f7c762a4cb4b9d1af5a67b93445fd476
Gerrit URL: https://gerrit.gromacs.org/9056

#4 Updated by Mark Abraham 3 months ago

  • Related to Task #1862: Fully replace t_topology by gmx_mtop_t added

#5 Updated by Gerrit Code Review Bot 3 months ago

Gerrit received a related DRAFT patchset '1' for Issue #2847.
Uploader: Mark Abraham ()
Change-Id: gromacs~master~I9bdd288460cab719a69e74011befd06d2b43ac51
Gerrit URL: https://gerrit.gromacs.org/9061

#6 Updated by Mark Abraham 2 months ago

This will need a preliminary change in release-2019 to revert the changes to gmx solvate and gmx insert-molecules to not use TopologyInformation, because the appropriate fix for TopologyInformation for supporting the selection use cases breaks the aforementioned tools.

That's not nice, particularly because there were follow-up changes in the implementations of those tools, but at least we have good test coverage for the reversion, and new test cases for the selection fix.

#7 Updated by Mark Abraham 2 months ago

  • Target version set to 2019.1

#8 Updated by Gerrit Code Review Bot 2 months ago

Gerrit received a related patchset '1' for Issue #2847.
Uploader: Mark Abraham ()
Change-Id: gromacs~release-2019~I6e4900eea38b5c2e1b85b0d48427dabe26b09f9d
Gerrit URL: https://gerrit.gromacs.org/9145

#9 Updated by Mark Abraham 2 months ago

  • Status changed from New to Fix uploaded

#10 Updated by Mark Abraham 2 months ago

  • Status changed from Fix uploaded to Resolved

#11 Updated by Paul Bauer 2 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF