search

proteneer / timemachine

Differentiate all the things!
Other
140 stars 17 forks source link

Ensure permutation equivariance of `oe_assign_charges` #1397

Closed maxentile closed 1 month ago

maxentile commented 1 month ago

Intent: ensure oe_assign_charges(mol)[perm] == oe_assign_charges(mol[perm]) for any renumbering of atoms.

(related to https://github.com/openforcefield/openff-toolkit/issues/983 )

Initial approach: Apply CanonicalRankAtoms to rdkit mol before converting to oemol, calling into omega, .... This does not resolve the issue. (This confirms sensitivity to other factors, such as bond ordering.)

Revised approach: Toggle omega setting SetCanonOrder from False to True. (And recover initial atom ordering using {Set/Get}MapIdx in calling context.)

maxentile commented 1 month ago

Reordering just the atoms doesn't achieve the initial goal.

@badisa suggested to toggle SetCanonOrder https://docs.eyesopen.com/toolkits/python/omegatk/OEConfGenClasses/OEMolBuilderOptions.html#OEConfGen::OEMolBuilderOptions::SetCanonOrder (which also reorders bond records, and reorders atoms within each bond record) -- currently set to False in: https://github.com/proteneer/timemachine/blob/80e4aae47169688200a802612c48cc54a0ee1010/timemachine/ff/handlers/nonbonded.py#L70-L71

badisa commented 1 month ago

Reordering just the atoms doesn't achieve the initial goal.

@badisa suggested to toggle SetCanonOrder https://docs.eyesopen.com/toolkits/python/omegatk/OEConfGenClasses/OEMolBuilderOptions.html#OEConfGen::OEMolBuilderOptions::SetCanonOrder (which also reorders bond records, and reorders atoms within each bond record) -- currently set to False in:

https://github.com/proteneer/timemachine/blob/80e4aae47169688200a802612c48cc54a0ee1010/timemachine/ff/handlers/nonbonded.py#L70-L71

To clarify, I was not suggesting to toggle SetCanonOrder was pointing that out that SetCanonOrder also reorders bonds which this PR does not. This was because we saw that SetCanonOrder(True) resolved an issue we saw. I do not know if the bond ordering also impacts the charges, if it does we may want to also canonicalize the bond ordering.