Closed mattwthompson closed 3 years ago
@justinGilmer @daico007 @umesh-timalsina given the severity of this bug (not critical, but also not negligible), could we get a patch release?
@mattwthompson we are planning to do that either today or early tmr
Great - thanks!
Bug summary
This one goes a little deep, so buckle up. Since Foyer's force field class does not explicitly encode the mixing rule/combination rule, it's somewhat ambiguous how it is ultimately applied. If the force field is meant to encode Lorentz-Berthelot mixing rule, things should work well because that is ParmEd's default and OpenMM assumes that throughout using
openmm.NonbondedForce
. If using geometric mixing rules, I believe the normal flow is to callForcefield.apply()
on something and then use the setter of the resulting ParmEdStructure
, i.e.my_struct.combining_rule == "geometric"
. Then, ParmEd's writers will generally handle what needs to be handled. For example, writing out to a GROMACS topology file will setcomb-rule
appropriately (3 is geometric):This tells GROMACS to use geometric mixing rules when computing the cross-interactions for each atom type. A deep subtlety here is that the cross-interactions of 1-4 pairs have already been computed by ParmEd based on the values given to it from the
openmm.System
in thepmd.openmm.load_topology()
call here. To repeat,openmm.NonbondedForce
constructs everything assuming Lorentz-Berthelot mixing rules and encoding geometric mixing rules takes a little more work (https://github.com/openmm/openmm/issues/1859, or @ahy3nz has a deep dive that covers this point: https://ahy3nz.github.io/posts/2019/30/openmm2/). Included in anopenmm.System
is the exceptions (OpenMM language), also knows as adjusts (ParmEd/Amber? language) or pairs (GROMACS language). So the sigma values in the ParmEdStructure.adjusts
list have been pre-computed incorrectly.I think I've reproduced this below, but the resulting errors are small (0.02 kj/mol/molecule) that I could easily be making a mistake that invalidates my conclusions.
I don't think fixing this elegantly would be possible; the easiest way to stop the bleeding would probably involve adding behavior around here that, if using a geometric mixing rule, goes through each
adjust.type.sigma
instructure.adjusts
and overwrites the existing value with a freshly computed geometric mean. A long-term fix involves a lot of OpenMM hacking that I don't think ParmEd is designed to adjust; I can expand on that if desired but it's no small task. Explicitly storing the combing rule in Foyer's force field object would be necessary for any sustainable solution IMHO.Code to reproduce the behavior
In:
Out:
Software versions
python -c "import foyer; print(foyer.version)"
) 0.9.1python --version
)? 3.8.10