Project

General

Profile

Bug #1421

sc-coul manual description is incorrect

Added by Berk Hess over 3 years ago. Updated about 1 year ago.

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

Description

mdp_opt.html says:
sc-coul: (no)
Whether to apply the soft core free energy interaction transformation to the Columbic interaction of a molecule. Default is no, as it is generally more efficient to turn off the Coulomic interactions linearly before turning off the van der Waals interactions.

But with a single component lambda, which is (currently) by far the most common setup, it gets turned on (without note) when is was set to no or not set in the mdp file.
Note that I think we can't change this behavior, since that would make many of the FE setups out there produce incorrect results. But the manual should describe this behavior clearly.

Associated revisions

Revision a19ebef3 (diff)
Added by Berk Hess over 2 years ago

Added warning for unnecessary soft-core

The sc-coul mdp option is, presently, only active with lambda states.
grompp now issues a warning when using soft-core without Van der Waals
decoupling without lambda states.
Also fixed an incorrect twin-range grompp check with PME.

Refs #1421.

Change-Id: I0605fe0f735d69f96f478612a00434eccef6232f

Revision b89ea581 (diff)
Added by Mark Abraham about 2 years ago

Update reference warnings for new grompp check

The simple FE setup is one that we think is used often, and there the
warning is useful, and the test setups invoke such a setup, so we may
as well ensure that the tests check that that warning works.

Refs #1421, #1722

Change-Id: I0d974b50db4a4e0986815ec6c4064e3c4738e0ef

Revision 7d62d8f6 (diff)
Added by Mark Abraham about 2 years ago

Fixed recently-introduced couple-lambda warning message

The grompp warning was not issued correctly. Any simulation that ran
was unaffected.

Refs #1421
Fixes #1722

Change-Id: Ib582d3749c283dbb03504c5ec09f854c518e11a5

Revision 262bf922 (diff)
Added by Rossen Apostolov about 2 years ago

Added a note about sc-coul being auto turned on.

The documentation didn't mention that the soft-core potential
is automatically switched on in the case of a single component
lambda.

Fixes #1421.

Change-Id: Ic478b8b1a68c9ef08764946337f72008808bb2b3

History

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

Gerrit received a related patchset '1' for Issue #1421.
Uploader: Rossen Apostolov ()
Change-Id: Ic478b8b1a68c9ef08764946337f72008808bb2b3
Gerrit URL: https://gerrit.gromacs.org/3586

#2 Updated by Rossen Apostolov almost 3 years ago

  • Status changed from New to Fix uploaded

#3 Updated by Szilárd Páll over 2 years ago

GROMACS 4.6 broke the behavior of the original implementation and what's worse is that it did it without letting the user know. It's also unfortunate that it's been 10 months and the issue has never been addressed (not to mention that we ended up with tens of microseconds of useless data).

As mentioned on gerrit, IMO correcting the manual smells like trying to cover up an inconvenient mistake. Instead, I think we should make sure we let users know about it:
  • grompp should issue a warning and inform the user about the workaround (setting sc-alpha = 0);
  • we may want to issue an mdrun note too given that this bug has been present in the code since the first 4.6 release and in the nearly two years since there have probably been many tpr's generated with unintended settings;
  • announce the issue on the mailing lists ASAP.

While the bug does not result in incorrect results, it can and likely has caused huge efficiency decrease in users' simualtion. In our runs my preliminary estimate is that sampling efficiency can drop by up to 10x when using the published recommendation of employing linear lambda scaling for these soft-cored Coulombic interactions.

I believe that this is severe enough that it warrants a fix in 4.6 too (even if no 4.6.8 gets released) and announced on the users list sooner rather than later.

#4 Updated by Michael Shirts over 2 years ago

Can you be slightly more specific about mathematical form? I think one source of confusion is that there's a lot going on in the free energy code (several different individually correct but mutually incompatible ways of doing things), and exactly what is wrong not entirely clear (at least to me), since I haven't used the code in the way that would lead to the behavior your are describing.

I'll try to state what I understand let you fill in the ambiguities. Things I think I know:

1. This is something that happens when fep-lambda is used (not when coul-lambda and vdw-lambda are used).

Things I'm not certain about:

1. Whether vector of lambda states are set, or just a single lambda state (I'm not sure if this matters, but thought I'd check)
2. What sigma and epsilon are in in states A and B (this definitely matters in the code logic).
3. By sampling efficiency being broken do you mean that the dhdl's are very large? Or that the correlation times are changing? I can't physically picture.
4. What is the potential energy as a function of lambda that you want, and what is the potential energy as a function of lambda that you are getting. Do you want a soft-core electrostatics and lennard Jones simultaneously, or something different?

But with a single component lambda, which is (currently) by far the most common setup, it gets turned on (without note) when is was set to no or not set in the mdp file.

Szilárd Páll wrote:

GROMACS 4.6 broke the behavior of the original implementation and what's worse is that it did it without letting the user know. It's also unfortunate that it's been 10 months and the issue has never been addressed (not to mention that we ended up with tens of microseconds of useless data).

As mentioned on gerrit, IMO correcting the manual smells like trying to cover up an inconvenient mistake. Instead, I think we should make sure we let users know about it:
  • grompp should issue a warning and inform the user about the workaround (setting sc-alpha = 0);
  • we may want to issue an mdrun note too given that this bug has been present in the code since the first 4.6 release and in the nearly two years since there have probably been many tpr's generated with unintended settings;
  • announce the issue on the mailing lists ASAP.

While the bug does not result in incorrect results, it can and likely has caused huge efficiency decrease in users' simualtion. In our runs my preliminary estimate is that sampling efficiency can drop by up to 10x when using the published recommendation of employing linear lambda scaling for these soft-cored Coulombic interactions.

I believe that this is severe enough that it warrants a fix in 4.6 too (even if no 4.6.8 gets released) and announced on the users list sooner rather than later.

#5 Updated by Szilárd Páll over 2 years ago

Correction: I thought the issue was a silent change in behavior from 4.5 to 4.6 as well (as well as an inconsistency in the behavior), but it turns out that this is not the case because the sc-coul: (no) option did not exist in 4.5.

Hence, the bug is a lesser one, "only" an internal GROMACS inconsistency in the free energy implementation. Unless I'm mistaken, formulating a FEP setup for perturbing electrostatics interactions using the "old" couple_lambda/foreign_lambda settings will result in soft-core being applied (by default), while using the "new" setup, i.e. @coul_lambdas" to accomplish the same will result in no soft-core being applied to the electrostatics - which is obviously more efficient.

So my initial assessment that this is a serious problem was not entirely correct as the "old" behavior of always using soft-core if sc-alpha is specified has in fact been preserved, but only for the "old" setup. So I think this issue still warrants at least a grompp warning and clearly documented behavior because it can easily create confusion and wasted compute time, e.g. if one is using the new setup but grabs some older mpd's to reproduce some result and realizes after a long simulation that the results are not converging (because soft core is unexpectedly making the lambda coulomb dependence non-linear). As option encoded in the tpr are not overridden, no run-time warning is necessary, I think.

Does that make sense? Am I overlooking something?

#6 Updated by Gerrit Code Review Bot over 2 years ago

Gerrit received a related patchset '1' for Issue #1421.
Uploader: Berk Hess ()
Change-Id: I0605fe0f735d69f96f478612a00434eccef6232f
Gerrit URL: https://gerrit.gromacs.org/4222

#7 Updated by Mark Abraham over 2 years ago

Does the patch in Gerrit suit, Szilard?

#8 Updated by Michael Shirts over 2 years ago

OK, grading all done, some time to code.

What's supposed to happen is that it is supposed to use linear coulomb whenever c12 is nonzero at both ends. sc is very inefficient otherwise. Before 4.5, it didn't do this.

Szilard, can you be more specific about what is not happening? Note I'm about to start working on the files you uploaded, so I will likely find that soon, but confirmation would help to ensure we are in the right place.

#9 Updated by Michael Shirts over 2 years ago

Michael Shirts wrote:

Actually, no files to see what exactly the inputs are doing. . .

OK, grading all done, some time to code.

What's supposed to happen is that it is supposed to use linear coulomb whenever c12 is nonzero at both ends. sc is very inefficient otherwise. Before 4.5, it didn't do this.

Szilard, can you be more specific about what is not happening? Note I'm about to start working on the files you uploaded, so I will likely find that soon, but confirmation would help to ensure we are in the right place.

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

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

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

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

#12 Updated by Erik Lindahl almost 2 years ago

  • Status changed from Fix uploaded to Feedback wanted

Both of the prevous commits only said they "refs 1421". Michael, what remains to be done here?

#13 Updated by Rossen Apostolov almost 2 years ago

  • Status changed from Feedback wanted to Resolved
  • % Done changed from 0 to 100

#14 Updated by Erik Lindahl about 1 year ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF