Project

General

Profile

Bug #1858

compute globals should not have logic about which integrator is in use

Added by Mark Abraham about 4 years ago. Updated about 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Affected version - extra info:
Affected version:
Difficulty:
uncategorized
Close

Description

In 488464e7, we removed the iterative cases for the md-vv integrator. This exposed a bug that I suspect has been there ever since the first implementation of md-vv.

It affects users doing replica exchange at the first successful exchange (#1848). Successful exchange triggers an extra call to compute_globals() (line 969 of md.c) to re-calculate KE-like quantities. If the integrator is not one of the VV flavors, then we segfault in global_stat (line 229 of stat.cpp) because such integrators are (erroneously) hard coded to collect the force virial when it is not required by the caller (who passed a NULL for it to be stored in). This inadvertently worked before removing constraint iteration, because bFirstIterate was FALSE for this call to global_stat(), which meant that the erroneous dereference of fvir didn't happen.

It also affects mdrun -rerun because that uses the same call to compute_globals().

When I removed the iteration code, I made a mental approximation of "all integrators now have one iteration, so bFirstIterate can be assumed to be true" which is valid, except that this particular call to compute_globals() is unusual, and inadvertently relied on the fact that the flag to indicate that this was a first iterate was (of course) not set.

There was also a test for bTemp || !bVV in global_stat, which didn't hurt because we passed ekind and bTemp was true (in at least this call to compute_globals()). So we don't segfault... but whether we trash data we intended to keep is pretty much unknowable in the current state of do_md().

I think the correct design is for integrators to pass the flags they want, and the global communication code do only what's asked directly of it.

I'll upload a patch that fixes this for release-5-1, where the bug is reported.

There remains the issue of bEkinAveVel in global-stat() which can wait to be cleaned up in master branch.


Related issues

Related to GROMACS - Bug #1848: Segfault with replica exchange at first successful exchangeClosed
Related to GROMACS - Task #1793: cleanup of integration loopNew

Associated revisions

Revision ed86315f (diff)
Added by Mark Abraham about 4 years ago

Stop global communication depending on integrator

In 488464e7, we removed the iterative case for the md-vv
integrator. This exposed a bug that I suspect has been there ever
since the first implementation of md-vv. Whether we do reduction on
the force virial should depend only on whether the main loop asked for
it, because we're planning to calculate quantities that need it,
e.g. pressure. Further discussion on Redmine.

Fixes #1858

Change-Id: I0fbce62a9732dac186aec687445dcb848151c4fd

History

#1 Updated by Mark Abraham about 4 years ago

  • Target version set to 5.1.2

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

Gerrit received a related patchset '2' for Issue #1858.
Uploader: Mark Abraham ()
Change-Id: I0fbce62a9732dac186aec687445dcb848151c4fd
Gerrit URL: https://gerrit.gromacs.org/5371

#3 Updated by Mark Abraham about 4 years ago

  • Related to Bug #1848: Segfault with replica exchange at first successful exchange added

#4 Updated by Berk Hess about 4 years ago

In do_md without VV, compute_globals is always called with the temp flag on.
In minimize.c it's always off.
So apparently the temperature summation for minimization is harmless.
But that conditional should and can also be changed, although we should do that in master.

#5 Updated by Mark Abraham about 4 years ago

Berk Hess wrote:

In do_md without VV, compute_globals is always called with the temp flag on.
In minimize.c it's always off.
So apparently the temperature summation for minimization is harmless.
But that conditional should and can also be changed, although we should do that in master.

OK. I think we also over-use CGLO_PRESSURE

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

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

#7 Updated by Mark Abraham about 4 years ago

  • Related to Task #1793: cleanup of integration loop added

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

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

#9 Updated by Mark Abraham about 4 years ago

  • Status changed from New to Resolved

#10 Updated by Mark Abraham about 4 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF