Project

General

Profile

Bug #2879

Virial and pressure completely incorrect with SHAKE

Added by Berk Hess 5 months ago. Updated about 2 months ago.

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

Description

The SHAKE constraint contribution to the virial is off leading to a large error in the pressure. Since the error is so large, this likely would have not gone unnoticed.
The issue is due to incorrect incrementing of the scaled_lagrange_multiplier pointer.

This also indicates that our tests for SHAKE are inadequate.

Only version 2018 is affected, not 2016 or 2019.

2_dt_allbonds_216_meth_npt.tpr (18.3 KB) 2_dt_allbonds_216_meth_npt.tpr Berk Hess, 05/07/2019 03:49 PM

Associated revisions

Revision de3a2cac (diff)
Added by Mark Abraham 2 months ago

Revert "Fixed dvdlambda for SHAKE + FE"

This broke normal simulations with SHAKE and does not seem
to fix anything with FEP simulations either.

This reverts commit 33093601ff229ed8c8a40e73866a7ff351e8963b.

Fixes #2879

Change-Id: Ib6e9b4fe2b65811e9f64c37e842fd0ed9cdb055c

Revision 17967ef5 (diff)
Added by Berk Hess 2 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 Artem Zhmurov 5 months ago

Virial is not tested in SHAKE tests. We do test it in new LINCS/SHAKE test (in master branch), but only for the case of one bond. I was planning to improve that.

#2 Updated by Mark Abraham 4 months ago

Yes, for simplicity in implementing the old tests, I didn't consider the virial (and then never got back to it). I will look into solving this.

#3 Updated by Artem Zhmurov 4 months ago

I think I found the bug:

In bshakef(...) function in shakef.cpp, when the computations are split to blocks, all blocks are given the same pointer to save scaled Lagrange multipliers. I.e., the function vec_shakef(...) should have lam in its arguments, not scaled_lagrange_multiplier. lam is incremented in the loop over blocks, scaled_lagrange_multiplier is not. As a result, when virial components are computed, the Lagrange multipliers are convoluted with wrong rij vectors, giving the wrong result.

#4 Updated by Berk Hess 4 months ago

Thanks Artem, but I/we already knew this is the issue. This is what Mark changed when fixing dV/dlambda for SHAKE. We need to have both correct.

#5 Updated by Mark Abraham 4 months ago

Berk Hess wrote:

Thanks Artem, but I/we already knew this is the issue. This is what Mark changed when fixing dV/dlambda for SHAKE. We need to have both correct.

It's good to know that we will have test coverage for it when fixed!

#6 Updated by Artem Zhmurov 4 months ago

Mark Abraham wrote:

Berk Hess wrote:

Thanks Artem, but I/we already knew this is the issue. This is what Mark changed when fixing dV/dlambda for SHAKE. We need to have both correct.

It's good to know that we will have test coverage for it when fixed!

If I re-introduce the bug into the latest version and the tests indeed fail in a way it should. But this happens only after change 9193, which is CUDA version of LINCS (https://gerrit.gromacs.org/#/c/9193/). Did not want to make another separate patch set for the constraints test at the time. The question is: shall I extract the constraints test improvements for virial into a sepparate patch or we can wait until the CUDA LINCS will make it in?

#7 Updated by Mark Abraham 4 months ago

Artem Zhmurov wrote:

Mark Abraham wrote:

Berk Hess wrote:

Thanks Artem, but I/we already knew this is the issue. This is what Mark changed when fixing dV/dlambda for SHAKE. We need to have both correct.

It's good to know that we will have test coverage for it when fixed!

If I re-introduce the bug into the latest version and the tests indeed fail in a way it should. But this happens only after change 9193, which is CUDA version of LINCS (https://gerrit.gromacs.org/#/c/9193/). Did not want to make another separate patch set for the constraints test at the time. The question is: shall I extract the constraints test improvements for virial into a sepparate patch or we can wait until the CUDA LINCS will make it in?

No need to extract. If we need to use them to test a fix, then someone can do a hack job.

#8 Updated by Berk Hess 3 months ago

The issue is worse, some (or all?) systems with shake explode, likely due to 33093601ff229ed8c8a40e73866a7ff351e8963b

Simple test system attached.

#9 Updated by Mark Abraham 2 months ago

  • Status changed from New to Resolved

#10 Updated by Mark Abraham about 2 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF