Project

General

Profile

Task #2423

Task #1793: cleanup of integration loop

modernize constraints code

Added by Mark Abraham over 1 year ago. Updated about 1 year ago.

Status:
New
Priority:
Normal
Assignee:
Category:
core library
Target version:
Difficulty:
uncategorized
Close

Description

This touches several strategic objectives:

  • modularize the code better
  • use proper C++ classes
  • minimize run-time work passing lots of maybe-unused parameters to do-it-all functions
  • minimize development overhead from unrelated changes to types and names of variables that happen to be textually adjacent
  • support transition to better auto-tuning
  • provide a framework in which a task can be separated from its execution on piece(s) of hardware
  • support evolution of integrator framework
  • evolve towards the integrator loop building a task graph for subsequent execution, rather than itself being the point of execution

Pieces of work needed in the checklist below, roughly in order of what makes sensible git commits. (NB the checklist will be similar to lists for other code needing modernization - what else should go on this checklist?):


Checklist

  • reduce include file coupling
  • reduce code coupling - refactor to split / unite / wrap functionality as seems suitable
  • remove unused code
  • make existing code more const correct
  • name source files sensibly
  • remove extern C
  • remove typedef struct idiom
  • put code into gmx namespace
  • use enum classes rather than legacy enums as type codes
  • use bool rather than gmx_bool
  • move code out of mdlib legacy module into another - is update + constraints + coupling a whole module, or three whole modules?
  • break any inappropriate inter-module dependencies
  • make structs for parameters to integrator_t and evaluate_energy - makes future changes to types and names of variables less noisy
  • assign responsibility for ownership and lifetime
  • unify DD and non-DD code paths
  • convert structs to opaque (mostly already done for constraints)
  • convert containers within structs to suitable STL types
  • convert opaque structs to Pimpl-ed classes
  • write/convert comments as Doxygen
  • use data member names in classes that conform to style (e.g. camel case and use of underscores for data members)
  • later, avoid using type codes
  • use IMdpOptions interface for command line args
  • use a (newly designed) interface to interact with simulation reduction code (e.g. for reporting LINCS RMSD)
  • decide a formalism in which interactions with essential dynamics and pull code works well
  • reconsider propagation and handling of simulation instability issues
  • consider how to express organizing constraining different kinds of coordinates, with different implementations, on (in future) different kinds of hardware - policy classes?
  • unify the DD and non-DD calls to prepare the constraints for the "domain"

Related issues

Related to GROMACS - Bug #2434: wrong values accumulated to dvdlambda in SHAKE with FE calcsClosed

Associated revisions

Revision f1ea683f (diff)
Added by Mark Abraham over 1 year ago

Reorganized constraint code

Renamed constraints files after just their algorithm, not the original
programming language, which matches the test file that they
have. Separated header files for each algorithm with matching
names. Removed extern C. Minimized header dependencies. Removed
excessive use of struct keyword.

Removed some header declarations of functions declared static.

Refs #2423

Change-Id: If60ab052ae73ad981fa031c2db6b3e206b380f4c

Revision 21b21524 (diff)
Added by Mark Abraham over 1 year ago

Moved and removed constraint code

No functionality changes here.

Removed code that was formerly called by the now-removed
combination of Andersen T coupling and constraints.

Removed crattle declaration from header, and moved its definition to
before the point of use, as normal for functions with file static
linkage.

Moved constr_r_max and support functions to constraintrange.cpp,
because conceptually the functionality is independent of which
algorithm implements the constraints. The current implementation only
works with LINCS, but that's because SHAKE and DD can't be used
together.

Refs #2423

Change-Id: I9e4fa1a4bb57118ca17c55f312103439b38190e3

Revision e573f0b9 (diff)
Added by Mark Abraham over 1 year ago

Add more const to uses of t_commrec and t_inputrec

Also removed some pointless struct keywords.

Refs #2423

Change-Id: Iacc008fb11a31646e798364e253de322c16ccaee

Revision ad79f778 (diff)
Added by Mark Abraham over 1 year ago

Create gromacs/mdrun module and move code there

Improving how we dispatch the work for do_md and friends requires that
they all compile as part of libgromacs (or all outside of it). The
code currently in src/programs/mdrun is there simply because it hasn't
been moved from the old src/kernel layout, rather than through any
clear design, so we should move it now that we have a reason.

Other code from src/programs/mdrun now needs to move into libgromacs
also, but we currently don't have a module for miscellanous mdrun
features, so we're adding to the abuse of mdlib for now.

Refs #1793
Refs #2423

Change-Id: I4f9ad5d0bf6585675472b49b2d5654f588b7214e

Revision 1ab524ff (diff)
Added by Mark Abraham over 1 year ago

Reorganize energy evaluation for EM

Used aggregate initialized structs to simplify future refactoring.

Refs #1793, #2423

Change-Id: Iedb8bf3b4cb2d7f238c4b674ce459564a3f76a20

Revision c8c0605d (diff)
Added by Mark Abraham over 1 year ago

Remove use of where(), if DEBUG etc.

The where() debugging function causes run-time branches in
release-mode builds, and replaces functionality for which one should
use a debugger.

Other such preprocessing clutter should also go away. Since we don't
check whether any of it compiles or produces useful data, we may as
well delete it.

This coincidentally simplifies the call sigatures of some
functionality, too.

Retained some useful essentialdynamics debug code, which is now always
compiled, and written from setup code only at debug level 2.

Refs #2423
Fixes #2122

Change-Id: I2c60c162734f4c2ec1d56153853a36d28e6a66ff

Revision 15c96888 (diff)
Added by Mark Abraham over 1 year ago

Clean up constraints code

Moved constraint code into gmx namespace.

Used more forward declarations in both the old and new headers.

Removed typedef from type declarations.

Removed gmx_ prefix from some names, since it is no longer required
and we may as well avoid verbiage.

Renamed some types more consistently with newer coding styles, and
called them class now where they will be one shortly.

Added some const correctness.

Apparently this inspires uncrustify to change positions of some
comments. Go figure.

Replaced all gmx_bool in constraints code with bool.

Maths functions from vec.h (which are not in the top-level namespace)
now need namespace qualifications.

Added and fixed various Doxygen, which is primitive in some cases
where the code was particularly cryptic.

Noted policy on multiple authors within Doxygen comments.

Refs #2423

Change-Id: I41bf3a4b9a4fbbcb3a3a7a27dc922d563abedbcb

Revision acc3940a (diff)
Added by Mark Abraham over 1 year ago

Break apart update_constraints

There are four distinct kinds of work being done, and never was any
call to update_constraints doing all of them, so it's better to have a
group of functions, each of which do one thing, and the relevant ones
called. This also makes it simpler to express by returning fast that
when we don't have constraints, we do nothing.

Made the logic for whether this is a log or energy step match that of
the main MD loop. The old implementation may not have prepared for the
last step correctly when it was triggered by something other than the
nsteps inputrec value.

Removed a commment mentioning iteration, which is a feature
that was removed a while ago.

Removed some ancient debug dump output.

Refs #2423, #1793

Change-Id: I21c10826721ddc9a79a33b1dc75971a20d0855d9

Revision 5ae5bf42 (diff)
Added by Mark Abraham over 1 year ago

Refactor SD update

The former use of multiple boolean control variables made the logic
hard to follow. Without constraints, the two parts of the integrator
are fused, which is now expressed explicitly.

This means it is now clear that the second half of the sd update does
not compute anything from the foreces.

Removed some of the vestiges of the way we once had two SD
integrators.

Refs #2423, #1793

Change-Id: I39d4cd0b8568859220b3a592b138f3f4405f8991

Revision e384d530 (diff)
Added by Mark Abraham over 1 year ago

Make Constraints a proper class

Converted the opaque C struct to a pimpl-ed C++ class.

Numerous callers of constraint routines now don't have to pass
parameters that are embedded within the class at setup time,
e.g. for logging, communication, per-atom information,
performance counters.

Some of those parameters have been converted to use const references
per style, which requires various callers and callees to be modified
accordingly. In particular, the mtop utility functions that take const
pointers have been deprecated, and some temporary wrapper functions
used so that we can defer the update of code completely unrelated to
constraints until another time. Similarly, t_commrec is retained as a
pointer, since it also makes sense to change globally.

Made ConstraintVariable an enum class. This generates some compiler
warnings to force us to cover potential bug cases with fatal errors.
Used more complete names for some of the enum members.

Introduced a factory function to continue the design that constr is
nullptr when we're not using constraints.

Added some const correctness where it now became necessary.

Refs #2423

Change-Id: I7a3833489b675f30863ca37c0359cd3e950b5494

Revision 143ab3d6 (diff)
Added by Mark Abraham over 1 year ago

Separate flexible constraint counting

Functions should do one thing, particularly when the thing is only
needed some of the time and complicates returning multiple things.

Minimized scope of some variables, avoiding risky re-use of variables
with the same name.

Refs #2423

Change-Id: I1860447b4780e76c20a53964a2a946512689b872

Revision ddc3dd50 (diff)
Added by Mark Abraham over 1 year ago

Use more containers and views for constraints code

Lifetime and usage is clearer if we don't use raw pointers.

Refs #2423

Change-Id: I7d76e30acfd209baf2256efbd3c40290ed38caab

Revision ec536cfc (diff)
Added by Mark Abraham over 1 year ago

Use initializer lists for Lincs and Task

This permits us to have some constructors that work the same way that
snew used to do (ie. calloc), so that future cleanup can use more
vectors, etc.

Also made a destructor of Constraints::Impl so the Lincs object gets
freed.

Refs #2423

Change-Id: Ie4a2001556d04f620a9d5241a9e1da9024cb9ce1

Revision dd0560e4 (diff)
Added by Mark Abraham over 1 year ago

Call Constraints::setConstraints with other setup code

Refs #2423

Change-Id: Iebee644a133f60778fea9b8030b7b7bf9656ba82

History

#1 Updated by Mark Abraham over 1 year ago

https://gerrit.gromacs.org/#/q/status:open+project:gromacs+branch:master+topic:constraints-cleanup has several patches implementing early points on the checklist - more will follow. I haven't tagged this redmine issue on them, yet

#2 Updated by Mark Abraham over 1 year ago

  • Description updated (diff)

#3 Updated by Gerrit Code Review Bot over 1 year ago

Gerrit received a related patchset '3' for Issue #2423.
Uploader: Mark Abraham ()
Change-Id: gromacs~master~If60ab052ae73ad981fa031c2db6b3e206b380f4c
Gerrit URL: https://gerrit.gromacs.org/7608

#4 Updated by Gerrit Code Review Bot over 1 year ago

Gerrit received a related patchset '2' for Issue #2423.
Uploader: Mark Abraham ()
Change-Id: gromacs~master~I9e4fa1a4bb57118ca17c55f312103439b38190e3
Gerrit URL: https://gerrit.gromacs.org/7609

#5 Updated by Gerrit Code Review Bot over 1 year ago

Gerrit received a related patchset '2' for Issue #2423.
Uploader: Mark Abraham ()
Change-Id: gromacs~master~Iacc008fb11a31646e798364e253de322c16ccaee
Gerrit URL: https://gerrit.gromacs.org/7646

#6 Updated by Gerrit Code Review Bot over 1 year ago

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

#7 Updated by Mark Abraham over 1 year ago

  • Description updated (diff)
  • Assignee set to Mark Abraham
  • Target version set to 2019

#8 Updated by Mark Abraham over 1 year ago

  • Related to Bug #2434: wrong values accumulated to dvdlambda in SHAKE with FE calcs added

#9 Updated by Gerrit Code Review Bot over 1 year ago

Gerrit received a related DRAFT patchset '21' for Issue #2423.
Uploader: Christian Blau ()
Change-Id: gromacs~master~I69460963927249c7d713e76f166e6c87122c68de
Gerrit URL: https://gerrit.gromacs.org/6020

#10 Updated by Gerrit Code Review Bot over 1 year ago

Gerrit received a related DRAFT patchset '1' for Issue #2423.
Uploader: Christian Blau ()
Change-Id: gromacs~master~Ic6ee2a71f5c5c11e161fa26aafbed2764e014daa
Gerrit URL: https://gerrit.gromacs.org/7680

#11 Updated by Gerrit Code Review Bot over 1 year ago

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

#12 Updated by Gerrit Code Review Bot over 1 year ago

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

#13 Updated by Gerrit Code Review Bot over 1 year ago

Gerrit received a related patchset '2' for Issue #2423.
Uploader: Mark Abraham ()
Change-Id: gromacs~master~I41bf3a4b9a4fbbcb3a3a7a27dc922d563abedbcb
Gerrit URL: https://gerrit.gromacs.org/7611

#14 Updated by Gerrit Code Review Bot over 1 year ago

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

#15 Updated by Gerrit Code Review Bot over 1 year ago

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

#16 Updated by Gerrit Code Review Bot over 1 year ago

Gerrit received a related patchset '2' for Issue #2423.
Uploader: Mark Abraham ()
Change-Id: gromacs~master~I7a3833489b675f30863ca37c0359cd3e950b5494
Gerrit URL: https://gerrit.gromacs.org/7612

#17 Updated by Gerrit Code Review Bot over 1 year ago

Gerrit received a related patchset '3' for Issue #2423.
Uploader: Mark Abraham ()
Change-Id: gromacs~master~Iebee644a133f60778fea9b8030b7b7bf9656ba82
Gerrit URL: https://gerrit.gromacs.org/7970

#18 Updated by Gerrit Code Review Bot over 1 year ago

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

#19 Updated by Gerrit Code Review Bot over 1 year ago

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

#20 Updated by Gerrit Code Review Bot over 1 year ago

Gerrit received a related patchset '2' for Issue #2423.
Uploader: Mark Abraham ()
Change-Id: gromacs~master~Ie4a2001556d04f620a9d5241a9e1da9024cb9ce1
Gerrit URL: https://gerrit.gromacs.org/7989

#21 Updated by Mark Abraham about 1 year ago

  • Target version changed from 2019 to future

Also available in: Atom PDF