Project

General

Profile

Feature #1975

grompp should warn if .mdp define does nothing

Added by Mark Abraham about 3 years ago. Updated 24 days ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
preprocessing (pdb2gmx,grompp)
Target version:
Difficulty:
uncategorized
Close

Description

Users sometimes misuse -DPOSRES_WATER for -DPOSRE_WATER, etc. and don't get the simulation they intended

Developers sometimes misuse -DDIHS for -DDIH, leading to tests that don't test dihedrals.

A helpful diagnostic for these cases would be that an .mdp whose include section defines something that was never used triggers a warning in grompp. Implementing that would require some basic parsing of that field for -D usage (or perhaps -I also), and something that bumps a counter when it parses an ifdef.

None of this should be done if we're switching to some new format for topologies and thus parsing, but we may not achieve that in practice for GROMACS 2017.

Whether in a new implementation or not, I further suggest that we drop the feature that lets you control the parsing of the topology from the .mdp file. Only having to update the equilibration.mdp file to produce the simulation.mdp file is convenient if you remember to do the right things, but clearly also brittle. pdb2gmx can just as easily write equilibration.top that has optional position restraints that users can turn on by editing the file. Having everything related to how the potential energy function works in the topology is conceptually attractive, but the only advantage I have thought of is usability. If so, the .mdp file is now clearly a higher-level thing that uses it.

Associated revisions

Revision e063041e (diff)
Added by Mark Abraham 6 months ago

Remove defines that tests don't intend to use

Implementing a check that mdrun will implement the simulation the user
asked for means we need our tests to specify their intent correctly.

Refs #1975

Change-Id: I041c0342c02406c1a9899ee397af5ccfb012f9a4

Revision 2363b5ff (diff)
Added by Kevin Boyd 5 months ago

Add grompp check for usage of "define" field in mdp

Users can misspell the strings specified with -D in the topology. This now
check that every -D field has a corresponding #ifdef and/or #ifndef and/or
C macro-style text replacement in the topology.

Fixed one unit test with unnecessary define field

refs #1975

Change-Id: Ie6329b234a60dde8efc34fb788e6296f241651ed

History

#1 Updated by Mark Abraham almost 2 years ago

  • Target version changed from 2018 to future

#2 Updated by Kevin Boyd 6 months ago

  • Status changed from New to In Progress
  • Assignee set to Kevin Boyd

#3 Updated by Kevin Boyd 6 months ago

I think this is worthwhile, even if our implementation changes in time for Gromacs 2020 - we could still put it in 2019.1 and 2018.x (or 2019, but IDK if something like this can go in so close to release). A check like this would have saved me some grief in the past.

#4 Updated by Gerrit Code Review Bot 6 months ago

Gerrit received a related patchset '1' for Issue #1975.
Uploader: Kevin Boyd ()
Change-Id: gromacs~master~Ie6329b234a60dde8efc34fb788e6296f241651ed
Gerrit URL: https://gerrit.gromacs.org/8882

#5 Updated by Kevin Boyd 6 months ago

  • Status changed from In Progress to Closed
  • Target version changed from future to 2020

#6 Updated by Kevin Boyd 6 months ago

  • Status changed from Closed to Fix uploaded

#7 Updated by Mark Abraham 6 months ago

Mark Abraham wrote:

Developers sometimes misuse -DDIHS for -DDIH, leading to tests that don't test dihedrals.

NB this really happened, fixed in regressiontests repo commit cd976e941b3a80adca1881c1461ad7fdff4f1fdd

#8 Updated by Gerrit Code Review Bot 6 months ago

Gerrit received a related patchset '1' for Issue #1975.
Uploader: Mark Abraham ()
Change-Id: regressiontests~master~I041c0342c02406c1a9899ee397af5ccfb012f9a4
Gerrit URL: https://gerrit.gromacs.org/8945

#9 Updated by Kevin Boyd 24 days ago

  • Status changed from Fix uploaded to Closed

Also available in: Atom PDF