compute globals should not have logic about which integrator is in use - Redmine #1858
In commit: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.
(from redmine: issue id 1858, created on 2015-11-23 by mark.j.abraham, closed on 2016-01-12)
- Relations:
- relates #1848 (closed)
- relates #1793 (closed)
- Changesets:
- Revision ed86315f by Mark Abraham on 2015-12-11T08:21:23Z:
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