search

openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io
http://openforcefield.org
MIT License
319 stars 93 forks source link

ForceField round trip changes Angle parameterize field #1672

Open ntBre opened 1 year ago

ntBre commented 1 year ago

Describe the bug Parsing a ForceField and rewriting it a to file changes the parameterize field on some Angles from k to 1 * boltzmann_constant ** 1, upsetting ForceBalance.

To Reproduce I needed to remove the Constraints section of a ForceField for ForceBalance and got this suggestion from Lily:

from openff.toolkit import ForceField

ff = ForceField("input.offxml", allow_cosmetic_attributes=True)
ff.deregister_parameter_handler("Constraints")
ff.to_file("output.offxml")

But I can reproduce it even with the deregister_parameter_handler line commented out. The changes are clearly seen when running diff input.offxml output.xml or even just grepping for boltzmann in both files.

Output

--- input.offxml        2023-07-20 17:08:33.120707559 -0400
+++ output.offxml       2023-07-20 17:22:05.578948496 -0400
@@ -115,8 +115,8 @@
         <Angle smirks="[*;r6:1]~;@[*;r5:2]~;@[*;r5;x2:3]" angle="126.74947883718123 * degree ** 1" k="165.02172217717344 * kilocalorie_per_mole ** 1 * radian ** -2" id="a13" parameterize="k, angle"></Angle>
         <Angle smirks="[*:1]~;!@[*;X3;r5:2]~;@[*;r5:3]" angle="124.76218228987901 * degree ** 1" k="102.212672475332 * kilocalorie_per_mole ** 1 * radian ** -2" id="a14" parameterize="k, angle"></Angle>
         <Angle smirks="[#8X1:1]~[#6X3:2]~[#8:3]" angle="123.69384605432525 * degree ** 1" k="126.52435713215367 * kilocalorie_per_mole ** 1 * radian ** -2" id="a15" parameterize="k, angle"></Angle>
-        <Angle smirks="[*:1]~[#6X2:2]~[*:3]" angle="178.0651310592217 * degree ** 1" k="99.85352234417054 * kilocalorie_per_mole ** 1 * radian ** -2" id="a16" parameterize="k"></Angle>
-        <Angle smirks="[*:1]~[#7X2:2]~[*:3]" angle="176.02345454674762 * degree ** 1" k="89.9604117556478 * kilocalorie_per_mole ** 1 * radian ** -2" id="a17" parameterize="k"></Angle>
+        <Angle smirks="[*:1]~[#6X2:2]~[*:3]" angle="178.0651310592217 * degree ** 1" k="99.85352234417054 * kilocalorie_per_mole ** 1 * radian ** -2" id="a16" parameterize="1 * boltzmann_constant ** 1"></Angle>
+        <Angle smirks="[*:1]~[#7X2:2]~[*:3]" angle="176.02345454674762 * degree ** 1" k="89.9604117556478 * kilocalorie_per_mole ** 1 * radian ** -2" id="a17" parameterize="1 * boltzmann_constant ** 1"></Angle>
         <Angle smirks="[*:1]~[#7X4,#7X3,#7X2-1:2]~[*:3]" angle="113.18673214011471 * degree ** 1" k="249.16533568188814 * kilocalorie_per_mole ** 1 * radian ** -2" id="a18" parameterize="k, angle"></Angle>
         <Angle smirks="[*:1]@-[r!r6;#7X4,#7X3,#7X2-1:2]@-[*:3]" angle="98.4609417604599 * degree ** 1" k="215.46610831911238 * kilocalorie_per_mole ** 1 * radian ** -2" id="a18a" parameterize="k, angle"></Angle>
         <Angle smirks="[#1:1]-[#7X4,#7X3,#7X2-1:2]-[*:3]" angle="109.26647374981178 * degree ** 1" k="107.59800871001211 * kilocalorie_per_mole ** 1 * radian ** -2" id="a19" parameterize="k, angle"></Angle>
@@ -128,7 +128,7 @@
         <Angle smirks="[#1:1]-[#7X2+0:2]~[*:3]" angle="111.61555367908721 * degree ** 1" k="104.67457969782329 * kilocalorie_per_mole ** 1 * radian ** -2" id="a24" parameterize="k, angle"></Angle>
         <Angle smirks="[#6,#7,#8:1]-[#7X3:2](~[#8X1])~[#8X1:3]" angle="117.56973608432911 * degree ** 1" k="145.03189317662233 * kilocalorie_per_mole ** 1 * radian ** -2" id="a25" parameterize="k, angle"></Angle>
         <Angle smirks="[#8X1:1]~[#7X3:2]~[#8X1:3]" angle="124.45512289998524 * degree ** 1" k="140.35006169568325 * kilocalorie_per_mole ** 1 * radian ** -2" id="a26" parameterize="k, angle"></Angle>
-        <Angle smirks="[*:1]~[#7X2:2]~[#7X1:3]" angle="175.2204655094496 * degree ** 1" k="117.562291770754 * kilocalorie_per_mole ** 1 * radian ** -2" id="a27" parameterize="k"></Angle>
+        <Angle smirks="[*:1]~[#7X2:2]~[#7X1:3]" angle="175.2204655094496 * degree ** 1" k="117.562291770754 * kilocalorie_per_mole ** 1 * radian ** -2" id="a27" parameterize="1 * boltzmann_constant ** 1"></Angle>
         <Angle smirks="[*:1]-[#8:2]-[*:3]" angle="111.96993713031328 * degree ** 1" k="249.99419564046633 * kilocalorie_per_mole ** 1 * radian ** -2" id="a28" parameterize="k, angle"></Angle>
         <Angle smirks="[#6X3,#7:1]~;@[#8;r:2]~;@[#6X3,#7:3]" angle="109.19365977372304 * degree ** 1" k="337.3883162802198 * kilocalorie_per_mole ** 1 * radian ** -2" id="a29" parameterize="k, angle"></Angle>
         <Angle smirks="[*:1]-[#8X2+1:2]=[*:3]" angle="122.99067282760342 * degree ** 1" k="324.48394466290887 * kilocalorie_per_mole ** 1 * radian ** -2" id="a30"></Angle>

Computing environment (please complete the following information):

mattwthompson commented 1 year ago

I don't recall if cosmetic attributes are meant to be serialized out - the docstring of ForceField.to_file implies they are by default.

Is this an issue with just AngleHandler and/or parameterize? Does the same thing happen with other cosmetic attributes?

lilyminium commented 1 year ago

I don't recall if cosmetic attributes are meant to be serialized out - the docstring of ForceField.to_file implies they are by default.

Yes, having discard_cosmetic_attributes=False by default implies to me that they should be written out as faithfully as possible.

Does the same thing happen with other cosmetic attributes?

This feels like more of #1633 and #1493, where I think the current fix so far only safeguards specific ParameterAttributes so everything else, including cosmetics, is fair game.

>>> from openff.toolkit import ForceField
>>> ff = ForceField("/Users/lily/Downloads/input.offxml", allow_cosmetic_attributes=True)
>>> handler = ff.get_parameter_handler("Angles")
>>> handler.parameters[15]._parameterize
1 boltzmann_constant
>>> handler.parameters[14]._parameterize
'k, angle'

This only happens with parameters where parameterize="k", as the toolkit correctly interprets parameterize="k, length" as a string.

j-wags commented 1 year ago

Agreed - The right resolution for this would be to not convert cosmetics - There's no safe way to encode that those should be quantities vs. strings, so we should always treat them as strings. I'll accept a PR that changes this, or will get to it myself after my current backlog!

lilyminium commented 1 year ago

There's #1637, but #1641 is still an issue.