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
318 stars 93 forks source link

More control in create_openmm_system() #1938

Closed jaketanderson closed 2 months ago

jaketanderson commented 2 months ago

Is your feature request related to a problem? Please describe. Currently, Interchange supports passing the following parameters into the creation of an OpenMM system:

combine_nonbonded_forces : bool, default=False
    If True, an attempt will be made to combine all non-bonded interactions into a single openmm.NonbondedForce.
    If False, non-bonded interactions will be split across multiple forces.
add_constrained_forces : bool, default=False,
    If True, add valence forces that might be overridden by constraints, i.e. call `addBond` or `addAngle`
    on a bond or angle that is fully constrained.
ewald_tolerance : float, default=1e-4
    The value passed to `NonbondedForce.setEwaldErrorTolerance`
hydrogen_mass : float, default=1.007947
    The mass to use for hydrogen atoms if not present in the topology. If non-trivially different
    than the default value, mass will be transferred from neighboring heavy atoms.

However, none of these parameters are exposed to toolkit.typing.engines.smirnoff.ForceField.create_openmm_system(): https://github.com/openforcefield/openff-toolkit/blob/747c2f1330be9adb972ecde7c892c237ed352f5b/openff/toolkit/typing/engines/smirnoff/forcefield.py#L1179-L1185

Instead, one of them is hardcoded as True and the others are simply left to their default values.

Describe the solution you'd like The four parameters currently available in Interchange could be exposed to create_openmm_system(). Ideally, one could then run ff.create_openmm_system(topology, ... , hydrogen_mass=3.024), making use of the functionality already provided by Interchange and OpenMM.

Describe alternatives you've considered Locally editing the create_openmm_system() function whenever I want to use HMR.

Additional context None that I can think of.

mattwthompson commented 2 months ago

I think our recommendation here is to chain together create_interchange(*args1).create_openmm_system(*args1) yourself, which allows you to pass through all of the fun stuff that is only explicitly supported in Interchange. (You can also freeze out the in-between with interchange = force_field.create_interchange(...).)

It may seem strange that the ~same functionality doesn't have all of the options exposed in both placed, but Interchange moves much more rapidly than the toolkit and changing very-public-facing API points like this one is something we try to do rarely.

j-wags commented 2 months ago

Yeah, this request isn't a bad idea, but it's just a bit more complexity than we want to maintain. create_openmm_system is largely a legacy call that we used primarily in the early days, and its purpose now is to keep old scripts running without changes. Advanced users are best off using create_interchange.

Even "little" API changes like this can snowball into big headaches later on, so I'm going to close this as "won't do" (but I do appreciate the well written issue!)

jaketanderson commented 2 months ago

I understand and agree with your reasoning, so I'll just continue the simple fix of making local edits when necessary. Thank you both!

j-wags commented 2 months ago

Oh, maybe I misunderstood. I was thinking that no local code changes would be needed by you, and anywhere you wanted to use

sys = ff.create_openmm_system(top, hydrogen_mass=3)

you could instead now use

interchange = ff.create_interchange(top)
sys = interchange.to_openmm_system(hydrogen_mass=3)

Does this work for your current needs? Happy to reopen this issue/hop on a call to clarify if you're unsure.

jaketanderson commented 2 months ago

@j-wags what you and Matt suggested works perfectly. My original confusion came from not realizing that I could directly create an interchange from a force field object, because up until recently my primary project was using a toolkit version that predated interchange. And then I made a second mistake when I glanced at Matt's suggested fix and somehow thought this was a change I had to make inside forcefield.py, as opposed to something I can do conveniently in my actual workflow.

Thanks for helping me correct these confusions, which came out of migrating my project from a very old toolkit version to the new-and-improved version. 😅 Had I known I could simply call create_interchange from the ff object, I probably wouldn't have thought it necessary to make this issue. Thanks again to both of you!