Task #1793: cleanup of integration loop
modernize constraints code
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?):
- 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"
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.
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
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
Add more const to uses of t_commrec and t_inputrec
Also removed some pointless struct keywords.
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.
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
Retained some useful essentialdynamics debug code, which is now always
compiled, and written from setup code only at debug level 2.
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.
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.
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
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,
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.
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.
Use more containers and views for constraints code
Lifetime and usage is clearer if we don't use raw pointers.
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
Also made a destructor of Constraints::Impl so the Lincs object gets
#1 Updated by Mark Abraham over 2 years 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