Project

General

Profile

Feature #1854

Remove all cyclic dependencies

Added by David van der Spoel about 4 years ago. Updated over 2 years ago.

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

Description

In order to make the code truly modular we would have to get rid of all cyclic dependencies. We check for those already during building but as a point of reference I thought it would be good to write down a hierarchy. In the long run every level in the hierarchy should only depend on the levels above. Before we reach this goal we have to remove legacyheaders (#1415). Trying to sketch the hierarchy I got stuck at level 3 already, where topology and fileio are cyclically dependent. So let us work on this and improve the modularity further.

0) external libraries, including system, thread_mpi, MPI etc.
1) utility
2) math
3) mdtypes, topology, fileio

Associated revisions

Revision e8e13a0e (diff)
Added by David van der Spoel about 4 years ago

Merged legacyheaders/types/state.h into mdtypes/state.h

Also merged swap/enum.h into legacyheaders/types/enums.h
to be moved later out of legacyheaders.
Part of #1415, #1854.

Change-Id: Ie53f190d6798b81c531896de80ef25c060836479

Revision 481dd0a9 (diff)
Added by Teemu Murtola about 4 years ago

Remove topology -> fileio dependency

There is no particular reason why .ndx reading/writing would need to use
gmxfio.h for I/O, instead of the simpler routines from futil.h. For
now, removing the use of gmxfio.h makes the dependencies much cleaner.

Part of #1854.

Change-Id: I86ed5dea88a230fbd337acb3a47048c9ab6e7fbe

Revision 76628f4e (diff)
Added by Teemu Murtola about 4 years ago

Move ifunc.* to topology/

This does not solve the main cyclic dependency that comes from
interaction_function definition depending on all the functions that
evaluate the interactions, but it does centralize this dependency to
topology: pbcutil -> gmxlib dependency is now removed.

Part of #1854.

Change-Id: Iaed05348cf663db7faeb7154a2838ea84d8bde1b

Revision 07367d2a (diff)
Added by Teemu Murtola about 4 years ago

Move disre.* and orires.* to listed-forces/

This removes topology -> gmxlib dependency, and moves code out of gmxlib
to a more appropriate location.

Convert existing comments to very rudimentary Doxygen documentation to
avoid Doxygen warnings.

Part of #1854.

Change-Id: I1aa56dd8f14eff90e035f1c69d59bb19a8dcb7ed

Revision 7b75d2e0 (diff)
Added by Teemu Murtola about 4 years ago

Create separate module for trajectory data

Move trx.h from fileio/ to a new trajectory/ module, and rename it to be
more descriptive. This directory can then evolve to have the basic data
structures and operations on trajectory information, similarly to the
topology module for structure information. This removes a
pbcutil <-> fileio cyclic dependency, and also makes the selection code
independent of fileio.

Moved energy.h from mdtypes/ to the same place, since it is used for
representing energy information during the trajectory. This makes
mdtypes at least slightly more mdrun-specific.

Part of #1854.

Change-Id: I4c266d78daefc3be9adefb56cd7cce8d9548634b

Revision b15c09b2 (diff)
Added by Teemu Murtola over 2 years ago

Remove pbcutil <-> mdtypes cyclic dependency

This may not be the cleanest solution, but it separates the
responsibilities of having knowledge of t_state and t_inputrec from
the low-level code of preserving the box shape. For a cleaner solution,
do_box_rel() could be separated into separate init and perform
functions, and having an internal data structure so that only the init
function would need parameters like the dimensions to correct.
Other alternative would be to move also do_box_rel() into state.cpp, but
not sure if there was a deeper reason putting it in pbcutil in the first
place.

Part of #1854.

Change-Id: I7ae16d3c2e795bd08d28da6111debf5d9d17599e

History

#1 Updated by Gerrit Code Review Bot about 4 years ago

Gerrit received a related patchset '1' for Issue #1854.
Uploader: David van der Spoel ()
Change-Id: Ie53f190d6798b81c531896de80ef25c060836479
Gerrit URL: https://gerrit.gromacs.org/5338

#2 Updated by Teemu Murtola about 4 years ago

Not sure if you know it, but we already have a visualization script for the dependencies, that also lays out the modules in a hierarchy. And it highlights all suppressed cycle edges in red, and ignored those in the layout. This is described in docs/dev-manual/gmxtree.rst. This probably provides a good starting point for thinking about the dependencies. And at least for the suppressions that I've put in docs/doxygen/cycle-suppressions.txt, I've put some thought into thinking about the dependency that I want to suppress, so working to remove those marked dependencies would provide one starting point.

#3 Updated by David van der Spoel about 4 years ago

Impressive stuff. Also scary. Looks like it will be a while before we make real inroads towards the goal of modularity.

#5 Updated by Mark Abraham about 4 years ago

We have funding though Horizon 2020 project BioExcel for the next 3 years that talks about "GROMACS will be turned into a state-of-the-art module-based C++ library." There's no specific requirements for us to fulfill, but it's going to get a slice of at least my time :-) Also as part of that project, CPMD wants to re-write its QM/MM implementation to call GROMACS force routines rather than GROMOS (for both performance and licensing reasons). Making that less impossible will be one of my personal foci, because the associated cleanup will surely help us in the long term.

(Also, Michael Shirts, Peter Kasson and I have an application under review that includes the objective of implementing a GROMACS API, which would also be thought to be very welcome for lots of folks.)

#6 Updated by Gerrit Code Review Bot about 4 years ago

Gerrit received a related patchset '1' for Issue #1854.
Uploader: Teemu Murtola ()
Change-Id: I86ed5dea88a230fbd337acb3a47048c9ab6e7fbe
Gerrit URL: https://gerrit.gromacs.org/5438

#7 Updated by Gerrit Code Review Bot about 4 years ago

Gerrit received a related patchset '1' for Issue #1854.
Uploader: Teemu Murtola ()
Change-Id: Iaed05348cf663db7faeb7154a2838ea84d8bde1b
Gerrit URL: https://gerrit.gromacs.org/5441

#8 Updated by Gerrit Code Review Bot about 4 years ago

Gerrit received a related patchset '1' for Issue #1854.
Uploader: Teemu Murtola ()
Change-Id: I1aa56dd8f14eff90e035f1c69d59bb19a8dcb7ed
Gerrit URL: https://gerrit.gromacs.org/5442

#9 Updated by Gerrit Code Review Bot about 4 years ago

Gerrit received a related patchset '1' for Issue #1854.
Uploader: Teemu Murtola ()
Change-Id: I4c266d78daefc3be9adefb56cd7cce8d9548634b
Gerrit URL: https://gerrit.gromacs.org/5444

#10 Updated by Teemu Murtola about 4 years ago

Changes up to https://gerrit.gromacs.org/5444 remove most of the cyclic dependencies from code below gmxlib (i.e., from most of non-mdrun code). The remaining incorrect dependencies are:
  • utility -> math: this gets removed in https://gerrit.gromacs.org/5259.
  • fileio -> gmxlib: this is only because of mdrun-specific checkpointing code is in fileio. Splitting checkpoint.cpp to cptio.* for reading/writing the actual file and to checkpoint.cpp that does the actual checkpointing, and moving the latter to, e.g., mdlib, should solve this cleanly.
  • pbcutil -> mdtypes: it would be nice to remove this by moving boxutilities.cpp somewhere else; the other few uses of t_inputrec are probably easy to remove.
  • topology -> listed-forces: this would require splitting interaction_function such that basic information about the interactions is in topology/, but the list of function pointers is in listed-forces. Long-term, it would be nice to make this more modular such that the list of interactions is constructed just in one place, from objects that define each interaction.

Anything above gmxlib doesn't make much sense at this point, but if the group kernels are really going to disappear in O(weeks), it will probably simplify things substantially there. Probably the next step after removing the kernels would be getting rid of whole gmxlib, moving the content to mdlib or some separate utility module(s).

#11 Updated by Gerrit Code Review Bot over 2 years ago

Gerrit received a related patchset '1' for Issue #1854.
Uploader: Teemu Murtola ()
Change-Id: gromacs~master~I7ae16d3c2e795bd08d28da6111debf5d9d17599e
Gerrit URL: https://gerrit.gromacs.org/6659

Also available in: Atom PDF