Project

General

Profile

Bug #2434

wrong values accumulated to dvdlambda in SHAKE with FE calcs

Added by Mark Abraham almost 2 years ago. Updated almost 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
core library
Target version:
Affected version - extra info:
all 5.1.x and 2016.x before 2016.6
Affected version:
Difficulty:
uncategorized
Close

Description

513813b514 refactored bshakef() to remove the use of lam as a temporary iteration variable. This was wrong, but worked fine for non-FE calcs. FE calcs will have accumulated arbitrary values to dvdlambda.

This could have been prevented in many ways, including declaring the original parameter as

 real *const lagr

and having SHAKE tests that cover a wider scope than cshake().


Related issues

Related to GROMACS - Task #2423: modernize constraints codeNew

Associated revisions

Revision 33093601 (diff)
Added by Mark Abraham almost 2 years ago

Fixed dvdlambda for SHAKE + FE

Garbage values have been accumulated to dvdlambda whenever SHAKE was
used.

Refactoring in 513813b514 removed the lam variable. Previously, it was
used as a temporary iteration variable over SHAKE blocks, but that
refactoring to use the main variable changed the logic of the
following loop.

Fixes #2434

Change-Id: Ia9642f50358ab31ec98f8317f7a1dcda8622a9f1

Revision 17967ef5 (diff)
Added by Berk Hess 7 months ago

Fix SHAKE dH/dlambda contributions

The constraint contributions to dH/dlambda would be incorrect
with more than one SHAKE block.

Refs #2434 abd #2879

Change-Id: I0cb30a9f61893ce57d76bac34e7352fe307efe4e

History

#1 Updated by Mark Abraham almost 2 years ago

  • Target version changed from 2016.6 to 2018.1

For now, we should fix in 2018.1, but we should back port to release-2016 also.

#2 Updated by Mark Abraham almost 2 years ago

Edit: this test case probably doesn't enter the relevant code that would show a bug

regressiontests freeenergy/simtemp dhdl.xvg output of 2016.2:

@    title "dH/d\xl\f{} and \xD\f{}H" 
@    xaxis  label "Time (ps)" 
@    yaxis  label "dH/d\xl\f{} and \xD\f{}H (kJ/mol [\xl\f{}]\S-1\N)" 
@TYPE xy
@ subtitle "\xl\f{} state 1:  = " 
@ view 0.15, 0.15, 0.75, 0.85
@ legend on
@ legend box on
@ legend loctype view
@ legend 0.78, 0.8
@ legend length 2
@ s0 legend "Thermodynamic state" 
@ s1 legend "Total Energy (kJ/mol)" 
@ s2 legend "\xD\f{}H \xl\f{} to T = 300 (K)" 
@ s3 legend "\xD\f{}H \xl\f{} to T = 311.787 (K)" 
@ s4 legend "\xD\f{}H \xl\f{} to T = 324.037 (K)" 
@ s5 legend "\xD\f{}H \xl\f{} to T = 336.768 (K)" 
@ s6 legend "\xD\f{}H \xl\f{} to T = 350 (K)" 
0.0000    1 -29093.266 -255.84591 0.0000000 265.89774 542.24302 829.44569
0.0200    0 -29372.768 0.0000000 261.88091 534.05154 816.91553 1110.8932
0.0400    0 -29376.373 0.0000000 257.37702 524.86680 802.86604 1091.7878

And 2018 branch with the incoming fix:

@    title "dH/d\xl\f{} and \xD\f{}H" 
@    xaxis  label "Time (ps)" 
@    yaxis  label "dH/d\xl\f{} and \xD\f{}H (kJ/mol [\xl\f{}]\S-1\N)" 
@TYPE xy
@ subtitle "\xl\f{} state 1:  = " 
@ view 0.15, 0.15, 0.75, 0.85
@ legend on
@ legend box on
@ legend loctype view
@ legend 0.78, 0.8
@ legend length 2
@ s0 legend "Thermodynamic state" 
@ s1 legend "Total Energy (kJ/mol)" 
@ s2 legend "\xD\f{}H \xl\f{} to T = 300 (K)" 
@ s3 legend "\xD\f{}H \xl\f{} to T = 311.787 (K)" 
@ s4 legend "\xD\f{}H \xl\f{} to T = 324.037 (K)" 
@ s5 legend "\xD\f{}H \xl\f{} to T = 336.768 (K)" 
@ s6 legend "\xD\f{}H \xl\f{} to T = 350 (K)" 
0.0000    1 -29093.281 -255.84591 0.0000000 265.89774 542.24302 829.44569
0.0200    0 -29372.785 0.0000000 261.88089 534.05150 816.91547 1110.8931
0.0400    0 -29376.383 0.0000000 257.37712 524.86700 802.86634 1091.7882

In this trivial case (SHAKE on a single methane in water), the impact was negligible. So even if we were testing on the contents of dhdl.xvg, we might not have caught this.

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

Gerrit received a related patchset '1' for Issue #2434.
Uploader: Mark Abraham ()
Change-Id: gromacs~release-2018~Ia9642f50358ab31ec98f8317f7a1dcda8622a9f1
Gerrit URL: https://gerrit.gromacs.org/7642

#4 Updated by Mark Abraham almost 2 years ago

  • Status changed from New to Fix uploaded

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

Gerrit received a related patchset '4' for Issue #2434.
Uploader: Mark Abraham ()
Change-Id: gromacs~master~I09b7a719b58f8f8192762431edc139674eaf23ae
Gerrit URL: https://gerrit.gromacs.org/7610

#6 Updated by Mark Abraham almost 2 years ago

Actually that test case is never going to show the issue, since it does simulated tempering, rather than perturbing the system.

#7 Updated by Mark Abraham almost 2 years ago

  • Status changed from Fix uploaded to Resolved

#8 Updated by Mark Abraham almost 2 years ago

  • Status changed from Resolved to Closed

#9 Updated by Mark Abraham almost 2 years ago

  • Related to Task #2423: modernize constraints code added

Also available in: Atom PDF