Project

General

Profile

Bug #2879

Virial and pressure completely incorrect with SHAKE

Added by Berk Hess about 2 months ago. Updated about 1 month ago.

Status:
New
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.

History

#1 Updated by Artem Zhmurov about 2 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 about 1 month 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 about 1 month 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 about 1 month 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 about 1 month 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 about 1 month 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 about 1 month 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.

Also available in: Atom PDF