mdrun features to deprecate for 5.0 - Redmine #1292
There are a number of things that I think we should deprecate for 5.0. The reasons vary:
- if we’ve known for years they’ve been broken and we haven’t bothered to fix them, we may as well throw them out and re-implement if/when we care again
- if they don’t scale well
- if their author has abandoned them
People still have the option of using old code versions, or doing the work to update/re-implement. We cannot afford to deliver a high-performance, high-quality, kitchen-sink code base. In particular, we can’t afford to create all the test cases that have a chance of showing whether these things still work now, never mind whether they continue to work as we refactor key data structures. Neither do we want to invest time into keeping code compiling, or compromising the design of data structures to support code that is broken (and we maybe don’t even know what is broken under what conditions).
Here’s what Mark thinks should go, and some detail on why. Edit feedback so far looks like this.
1) Particle decomposition
Doesn’t support checkpointing. Doesn’t scale because it does not balance
load. Removing it will simplify a lot of conditionally executed code
paths. The main caveat is whether there are important algorithms whose
implementations still require it (see below). Erik is all for killing
PD
2) Fancy distance restraints
Peter and David are keen to retain most/all of the restraint
machinery
a) Ensemble-averaged distance restraints
Has been disabled since 2009. If/when it did work before that, it
required PD. Its implementation and that of REMD seem to compromise each
other a bit. Its implementation doesn’t look like it would work with
multiple ranks per simulation.
b) Time-averaged distance restraints
Disabled with DD since 2008. I can’t see any reason why it should
require PD if simple distance restraints work with DD. If the issue is
that DD only works with distance restraints that happen to be inside the
cutoff (so that the positions are available), then it should be not too
painful to add a mechanism to keep track of the handful of atom pairs
involved. However, the normal DD mechanism for communicating positions
is probably unsuited to communicating between domains on arbitrary
processors, so we’d have to add a special round of communication. That’d
scale poorly, but that’s the choice of the person using such restraints.
But I’d prefer to remove time-averaged distance restraints than to keep
PD just to support it.
c) Writing of pair distance and/or time-averaged pair distance to energy
files
Seems more like debugging output than anything you’d use in
post-processing (and whatever it does support could be implemented via a
trajectory-analysis tool).
3) Fancy orientation restraints
Pretty much as above. Stuff’s been disabled with DD for years.
As above, Peter and David are keen to retain
4) Current polarization support?
Is anything here known to be useful enough that we’d want to keep
it? David and Michael think Drude (a.k.a. shell) model should
definitely be retained, but perhaps via re-implementation.
5) QM support
No real beef here, but unless someone is willing to construct at least
some simple test cases, we won’t know whether it is broken. My
experience of asking for test cases from the authors of specialist
features has been mixed :-) Existing QMMM to be removed and
re-implemented. WIP in qmmm branch.
6) Energy minimizers
I’d lose L-BFGS if it causes any pain, since its implementation has
never worked in parallel or with constraints. Useful for deep
minimization before NMA. Consult with Erik if there are issues.
7) Generalized Born
It worked in 4.5, but various aspects of implicit solvation got broken
during the 4.6 cycle, and I see no pressure to fix any of it. The method
is anachronistic. Its implementation will not benefit from anything else
we are doing. Its existence leads people to assume that all the other
things GROMACS can do work with GB (e.g. free-energy calculations), and
I suspect that all that ever worked was the stuff that was wanted for
the Larsson and Bjelkmar publications. Preference from David and
Michael to retain to support niche market. Erik torn.
8) MTTK pressure coupling
Per Michael’s intentions. Retain, but remove support for
topologies with constraints.
9) Reaction-field-nec
Was only ever a backward-compatibility feature?
10) Encad-shift
We deprecated its force field, do we need its shift?
11) mdrun -ionize
David to remove
12) GCT
Believed broken. David to remove.
13) mdrun -seppot
NEW Looks to Michael, Teemu and Mark like debugging output, rather
than typical .log file output (see #1294). If so, we should either
write output only in response to -debug, or not write the output.
Please let your thoughts be known - but we can’t take more than a few weeks over this. Code has to change! In particular, there are things above that I am unsure about, and I’ll update the main list as I learn from you.
If you disagree with wanting to remove something and expect some debate, please fork a new Redmine, note your disagreement here, and link the two issues. Otherwise we might drown in the cross-discussion :-)
(from redmine: issue id 1292, created on 2013-06-26 by mark.j.abraham, closed on 2014-05-13)
- Relations:
- relates #1416 (closed)
- relates #753 (closed)
- relates #973 (closed)
- relates #1971 (closed)
- child #1429 (closed)
- child #1500 (closed)
- Changesets:
- Revision 8e2028cd by Mark Abraham on 2013-09-18T15:57:17Z:
Remove forcefield-scan feature
Refs #1292
Change-Id: I05d8dec45665d5dc3b72b72d312240abbd2983c7
- Revision 864de6d4 by Mark Abraham on 2013-09-26T08:30:56Z:
Remove General Coupling Theory stuff
Also update mdp_opt.html and src/contrib to no longer refer to
defunct xmdrun machinery. This commit will prevent compilation of
src/contrib/prfn.c, but that is slated for removal in I842a92ec41.
Refs #1292
Change-Id: I8e06d3395787545ae5aba5334acfc57b7d8683c3
- Revision 4282773f by Mark Abraham on 2013-09-26T12:40:20Z:
Remove mdrun -ionize feature
Refs #1292
Change-Id: Ic3b8bf33265304f903fcba749fe41d4f00386d1d
- Revision 30113ccc by Mark Abraham on 2013-10-17T22:11:09Z:
Move mdrun trajectory writing into wrapper function
Refs #1292, #1193, #1137
Change-Id: I3f19e0995ff7fab465184d5bab8c2683260af853
- Revision 8df8c14d by Mark Abraham on 2013-10-28T17:36:13Z:
Create fileio module
This patch contains only code motion. There are no functional code
changes. Moves lots of I/O code into src/gromacs/fileio. This means
lots of changes to avoid having everything as a compile-time dependency
of everything else because everything is pulled in via typedefs.h,
etc. Note that src/gromacs/legacyheaders/filenm.h and
src/gromacs/legacyheaders/types/filenm.h have been consolidated into
src/gromacs/fileio/filenm.h.
I/O code in files named stat*[ch] now lives in various new files in
fileio.
Files outside of the module now #include its header files in a proper
way, e.g. #include #include "../fileio/filenm.h" or
"gromacs/fileio/filenm.h" according to whether they are an installed
header, or not. Files within the module are blessed and do not need
that qualifier.
This module installs most of its headers (because they're almost all
inter-dependent; gmxfio_int.h is not installed because it is
only useful internally, vmdio.h is not installed because it
relies on a header from src/external)
Files in new module
* conform to preferred include-guard format.
* have up-to-date copyright headers thanks to Teemu's automatic
script
* that are installed headers refer to other GROMACS include files via
relative paths
Moves mdrun trajectory writing into wrapper function.
Removes small pieces of I/O code that was hiding behind "#if 0".
Some pieces of I/O code specific to the gmxpreprocess module have
remained there.
Moved a cppcheck suppression to follow matio.cpp to its new home.
Minor fix to xdrf.h logic, since it is now the subject of a CMake
test.
Refs #1292, #1193, #1137
Change-Id: I820036298d574966d596ab9e258ed8676e359184
- Revision 20fa2cda by Mark Abraham on 2014-02-20T18:03:01Z:
Remove particle decomposition
The paths that are eliminated are those for which MD_PARTDECOMP was
needed, those triggered by PARTDECOMP(cr), and any for which PAR(cr)
&& !cr->dd.
Reviewers, please note
* that for the purposes of this patch, OpenMP is not a form of
parallelism,
* the definition of DOMAINDECOMP(cr) in commrec.h,
* that TPI and NM can run in parallel, but use neither PD nor DD,
* multi-simulations run in parallel but need not use DD, so
* we still need both PAR(cr) and DOMAINDECOMP(cr), and
* I have generally left the indenting alone to make for easy review,
but we will uncrustify before merging
Summary of changes in this patch:
Removed
* code triggered by MD_PARTDECOMP
* code triggered by PARTDECOMP(cr)
* anything decomposition-related with "pd" in the name (but the name
clash with x86 vector intrinsics is unfortunate)
* t_mdatoms field called start (DD does atom number remapping)
Note that bPDvsitecomm was never set anyway!
* manual section
Renamed two functions with "pd" in their name changed to "serial"
because they are still needed there (and moved them?)
Deleted files, functions and function paramters that only supported PD
code paths.
Cleaned up
* use of DOMAINDECOMP(cr)
* a bit of Generalized Born code that should have been static or
was unused
* bcast_state machinery can now be much simpler; still does the right
thing, but does it further behind the scenes and easier to
understand
* made explicit the assumption of replica exchange that the
integrator is dynamical, which makes for some minor
simplifications
Refs #1292
Change-Id: If029f16e6b4b06d58d465afe072a3cde6481479e
- Revision d17542a3 by Mark Abraham on 2014-03-17T17:46:59Z:
House-keeping for MTTK
Leapfrog + MTTK silently produced a .tpr that is probably broken;
certainly the documentation only states support for Velocity-verlet
integrators.
Added note about deprecation and planned removal of MTTK + constraints.
Refs #1292
Change-Id: Iec2cf0dd866242735ce04a954e585b2461f6e701
- Revision 8b550151 by Mark Abraham on 2014-08-17T06:24:19Z:
Remove mdrun -seppot
This output of rank-local energies is not needed for any routine use,
and isn't much use in mdrun -debug either. Removing it simplifies a
few code paths, including removing some dependencies on writing sane
things to the log file. Eliminated some now-unused parameters.
Refs #1292
Change-Id: I7628b78862144ae2478b6f42a6818915e3a22fe3
- Revision 7c6185b4 by Mark Abraham on 2014-12-13T10:41:49Z:
Remove heuristic group-scheme neighbour-list updates
-1 == nstlist used to trigger the use of an algorithm that did
neighbour searching only when particles had moved far enough that
there might be a need to update the list. This supported obtaining
better energy conservation with the group cut-off scheme. It has been
superseded by the Verlet cut-off scheme.
Part of #1292
Change-Id: I074790c1c5670e874b8b587a1d46e62e2edc8961
- Revision 89012d48 by Mark Abraham on 2019-03-02T02:39:38Z:
Remove defunct mdrun options
These worked with General Coupling Theory, and were
not removed alongside that code when it was removed.
Refs #1292
Change-Id: Ia795ea81e7c38ba3b895f926ce048c0ac2dfba1a