Project

General

Profile

Task #1852

Remove group scheme

Added by Mark Abraham almost 4 years ago. Updated 5 months ago.

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

Description

We can do this readily once we're sure we have no tests that need the group scheme. Regressiontests have been ported, TPI tests have been disabled, if we find any others we can discuss.

At some future time, hopefully soon, we will
  • decide how to read tables from grompp or mdrun for the Verlet scheme (some work in Gerrit now)
  • got GPU and CPU kernels working for user tables (some work in Gerrit now, none for CPU)
  • got no-PBC simulations working for nxnxm (in Gerrit now)
  • test particle insertion working for nbnxm
  • FEP kernel ported to nbnxm-style pair lists
    Anything else?

We removed AdResS long ago.

Some suggested patches that might make sense to implement this are found in the checklist.

remove SIMD group scheme kernels
change handling of cutoff-scheme in grompp, so that mdp files for group scheme are rejected with a useful error message
make cutoff-scheme optional in mdp field (permit users to "choose" only Verlet, but ignore it when they do)
remove mdrun options for table input
add fatal error for pbc = no
add fatal error for TPI
change handling of t_inputrec.cutoff_scheme so mdrun issues a fatal error for group-scheme tpr
remove t_forcerec.cutoff_scheme, along with ecuts enum and all code paths called by ecuts_GROUP
clean up t_forcerec fields no longer used
clean up Ewald code no longer used
move t_inputrec.cutoff_scheme to end of t_inputrec, like other removed features
update docs to remove references to old cutoff schemes (e.g. rlist)
stop grompp doing anything with charge groups (and update docs to note that such topology fields are now ignored)


Checklist

  • remove SIMD group scheme kernels

Subtasks

Task #2422: write C kernel for tables in Verlet schemeNew

Related issues

Related to GROMACS - Feature #1837: Design of new table classesClosed
Related to GROMACS - Feature #1347: future of tablesNew
Related to GROMACS - Feature #1666: new approach for Verlet-scheme kernel generationNew
Related to GROMACS - Task #1971: Removing buggy features vs. keeping workflows New
Related to GROMACS - Feature #2931: Tables in Verlet kernelsNew

Associated revisions

Revision 85d918db (diff)
Added by Mark Abraham almost 4 years ago

Remove AdResS

This feature will disappear with the group scheme, so we might as well
get it out of the way first. Doing this removal on its own might help
re-implement some time, if someone was keen.

Removed bVir, bPress, bSurft fields of t_mdebin, because all MD
algorithms now support such calculations.

gmx grompp now issues a fatal error if the main adress .mdp option is
on, and otherwise ignores the obsolete fields (like we do with other
.mdp options we've removed).

mdrun can read old .tpr files, but issues a fatal error if AdResS was
active in them.

gmx dump and gmx compare ignore all AdResS related fields.

Other tools can still read such .tpr files for their other content.

Removed Sebastian Fritsch from GROMACS 2016 contributor list, since he
only worked on AdResS features. Christoph Junghans made other
contributions that are still useful, and so remains.

Removed obsolete literature references

Also fixed some incorrect doxygen of init_forcerec().

Part of #1852

Change-Id: I22fa0fe480148aeda0ace194646a5ec2f3d20a8c

Revision 65e33d97 (diff)
Added by Mark Abraham over 3 years ago

Separate table construction

Construction of tables for the group scheme, pair interactions and
dispersion correction are now separated. The resulting tables are
never re-used for something else. This uses slightly more memory, but
makes the logic rather more simple. Some of the tables are now held by
reference by their owners, rather than by value, which might improve
cache locality a little.

With this change, we can implement the table support for the Verlet
scheme without getting involved with the group-scheme code, and will
have an easier time removing the group scheme.

Refs #1666, #1852

Change-Id: I8ca608f0e41b02723e6080b80b04d9e7ff048900

Revision d89dbd07 (diff)
Added by Paul Bauer 9 months ago

Change shell minimzation nm calculation to verlet

To make sure that shell minimization code works with the Verlet scheme
the test is moved to use that.

Part of #1852

Change-Id: I7c387a75438d2b6d8f97f5df83331a2008798e59

Revision 7ca9f08c (diff)
Added by Mark Abraham 7 months ago

Remove SIMD support from group scheme

It might take a bit longer to craft the patches that remove the rest
of the group scheme, but we may as well speed up our compilation times
first.

Refs #1852

Change-Id: I082e40e04678c744d9da8119333210878294a021

Revision ce17e81d (diff)
Added by Mark Abraham 5 months ago

Initial deactivation of group scheme.

mdrun now gives a fatal error with a group-scheme .tpr, so anybody
using one won't get a segfault.

do_force now only has one implementation, which suits those working
on improved force calculations.

More removal of inactive code will follow later. Noted TODO to fix the
release notes properly in such a commit.

Refs #1852

Change-Id: I3b13135565951f4d7f872ddf3b8518860eccfdb0

Revision 2d983f41 (diff)
Added by Szilárd Páll 4 months ago

Group scheme related cleanup

Removed:
- SR force calculation invocation
- LR correction invocations
- SR free energy component accumulation

Minor cleanup.

Refs #1852

Change-Id: I4e22986279039a0f49a5a8be3c447f4115308492

Revision b62fcc55 (diff)
Added by Kevin Boyd about 2 months ago

Remove group scheme checks from runner

refs #1852

Change-Id: I906ffc7c063694fbbd17128ee0d1e62259040306

History

#1 Updated by Mark Abraham almost 4 years ago

We're not likely to have any kind of twin-range / MTS support by the time we ship GROMACS 2016, so there's a bunch of old .tprs that can no longer be run. I don't think it is wise to run pre-4.6 .tpr files that happen to use a single-range scheme that happens to fit the requirements of the Verlet scheme, because any such simulations were semantically different (no buffer) and can't be compared with new ones. Defaulting them to a no-buffer version of the Verlet scheme is not terrible, but such simulations are arguably wrong, and they still always get a potential shift and so aren't readily comparable. So, I think there is a serious case for mdrun in GROMACS 2016 to refuse to run all pre-4.6 .tpr files. I see this very much as a step in the direction of "correct by default," and I'd certainly put that objective before "forward compatible .tpr files." (Of very much secondary concern is that this lets us remove small amounts of code that supports reading such files.)

#2 Updated by Gerrit Code Review Bot almost 4 years ago

Gerrit received a related patchset '1' for Issue #1852.
Uploader: Mark Abraham ()
Change-Id: I22fa0fe480148aeda0ace194646a5ec2f3d20a8c
Gerrit URL: https://gerrit.gromacs.org/5311

#3 Updated by Gerrit Code Review Bot over 3 years ago

Gerrit received a related patchset '8' for Issue #1852.
Uploader: Mark Abraham ()
Change-Id: I8ca608f0e41b02723e6080b80b04d9e7ff048900
Gerrit URL: https://gerrit.gromacs.org/5132

#4 Updated by Mark Abraham over 3 years ago

#5 Updated by Mark Abraham over 3 years ago

#6 Updated by Mark Abraham over 3 years ago

  • Related to Feature #1666: new approach for Verlet-scheme kernel generation added

#7 Updated by Mark Abraham about 3 years ago

  • Target version changed from 2016 to 2018

#8 Updated by Mark Abraham about 3 years ago

  • Target version changed from 2018 to 2016

Mark Abraham wrote:

We're not likely to have any kind of twin-range / MTS support by the time we ship GROMACS 2016, so there's a bunch of old .tprs that can no longer be run. I don't think it is wise to run pre-4.6 .tpr files that happen to use a single-range scheme that happens to fit the requirements of the Verlet scheme, because any such simulations were semantically different (no buffer) and can't be compared with new ones. Defaulting them to a no-buffer version of the Verlet scheme is not terrible, but such simulations are arguably wrong, and they still always get a potential shift and so aren't readily comparable. So, I think there is a serious case for mdrun in GROMACS 2016 to refuse to run all pre-4.6 .tpr files. I see this very much as a step in the direction of "correct by default," and I'd certainly put that objective before "forward compatible .tpr files." (Of very much secondary concern is that this lets us remove small amounts of code that supports reading such files.)

Note to self - find such a .tpr (from an old redmine?) and see what happens now.

#9 Updated by Mark Abraham about 3 years ago

  • Target version changed from 2016 to 2018

Mark Abraham wrote:

Note to self - find such a .tpr (from an old redmine?) and see what happens now.

Ran with the old regressiontests/complex/fe_test and got

Fatal error:
Twin-range simulations are no longer supported

which is fine for 2016.

By the time we remove the group scheme, then there will be no way to reliably run a group-scheme .tpr. I don't think we should write code to detect whether there's a buffer and whether its size is consistent with the current default buffers, and then remove charge groups and unilaterally apply a potential shift, etc. So in practice, .tpr files with versions before those of 4.6 will be unsupported once we remove the group scheme.

#10 Updated by Mark Abraham over 2 years ago

  • Target version changed from 2018 to future

unlikely to happen for 2017

#11 Updated by Roland Schulz over 2 years ago

I strongly suggest we remove the group kernels right after the 2017 release.

#12 Updated by Mark Abraham over 1 year ago

The Verlet scheme does not currently support energy group exclusions, though I suspect this is not a hard thing to implement, given the other kinds of exclusions currently supported.

#13 Updated by Roland Schulz over 1 year ago

I don't think we should wait on that. Wouldn't it have been added after 2 years if it was an essential feature? Users can always use the old version if they want to use it before it is added back (if it isn't added back before 2019 is released).

#14 Updated by Mark Abraham over 1 year ago

Roland Schulz wrote:

I don't think we should wait on that. Wouldn't it have been added after 2 years if it was an essential feature? Users can always use the old version if they want to use it before it is added back (if it isn't added back before 2019 is released).

At some point we are likely to have to accept that we won't make all things work. For example, IIRC membed depends on such exclusions. Given the range of alternative approaches for doing this (including using old GROMACS for membed), the relative ease of reimplementing it once we have more API like functionality working, and the amount of preliminary work needed now to be able to produce test cases that will show that the new implementation on Verlet exclusion stuff does all the same things that the old one does, the effort doesn't make much sense.

It's nice having been a community code that accepted lots of contributions, but we can't let that be an anchor around our collective neck.

#15 Updated by Mark Abraham 11 months ago

  • Description updated (diff)

TPI is only supported with the group scheme

#16 Updated by Mark Abraham 11 months ago

  • Related to Task #1971: Removing buggy features vs. keeping workflows added

#17 Updated by Roland Schulz 11 months ago

Is it going to be OK to remove them right after the 2019 branch is created?

#18 Updated by Erik Lindahl 11 months ago

Let's wait until we make the release, but then it's fine. The reason for waiting is that experience tells me we'll have a ton of patches during the beta phase of release-2019 that also need to be backported, so we don't want to huge divergence in the branches already during the beta.

#19 Updated by Mark Abraham 11 months ago

I'm happy with changes (in master) during the beta phase that don't involve a lot of code movement (e.g. enable c++14, stop group scheme kernel testing, update cmake version and use better implemetnations), but much less happy with general refactoring with many-LOC-but-minor changes that makes for merge complexity, or something like removing handling of charge groups from DD code.

#20 Updated by Gerrit Code Review Bot 9 months ago

Gerrit received a related patchset '2' for Issue #1852.
Uploader: Paul Bauer ()
Change-Id: regressiontests~release-2018~I7c387a75438d2b6d8f97f5df83331a2008798e59
Gerrit URL: https://gerrit.gromacs.org/8732

#21 Updated by Gerrit Code Review Bot 7 months ago

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

#22 Updated by Mark Abraham 7 months ago

I'm think that in master branch we might remove the SIMD and interaction-specific kernels, just leaving the generic one so that we can have it available while perhaps we want that while testing the new Verlet tables? Then again, it's ok to test that against 2019 branch.

#23 Updated by Mark Abraham 7 months ago

  • Description updated (diff)

https://gerrit.gromacs.org/#/c/8946/ disabled TPI testing until TPI has been ported to run with the Verlet scheme.

We've decided that we can't have all these big re-implement tasks blocking progress here, so I have edited the description accordingly.

#24 Updated by Mark Abraham 7 months ago

  • Description updated (diff)

Suggested subtasks now in the description. Some of them make sense to do in order from the top, some can start now.

We do want old .tpr files to be readable (e.g. as input for analysis tools) but never able to be used for computing forces.

Note that the free-energy kernel in src/gromacs/nonbonded is still used from the Verlet scheme for FEP, so it and supporting headers for data structures will have to remain for now.

#25 Updated by Gerrit Code Review Bot 7 months ago

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

#26 Updated by Mark Abraham 6 months ago

  • Description updated (diff)

removed checklist, were no longer using the redmine feature

#27 Updated by Mark Abraham 5 months ago

Just to be clear - we have made a decision to remove the group scheme for 2020. Ideally we will find time to replace e.g. table support for Verlet, but we can't have this legacy code in the way of multiple projects.

#28 Updated by Gerrit Code Review Bot 5 months ago

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

#29 Updated by Szilárd Páll 4 months ago

Also available in: Atom PDF