Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Combine force fields in-memory #1948

Open
mattwthompson opened this issue Oct 21, 2024 · 3 comments · May be fixed by #1996
Open

Combine force fields in-memory #1948

mattwthompson opened this issue Oct 21, 2024 · 3 comments · May be fixed by #1996
Labels
reliability

Comments

@mattwthompson
Copy link
Member

Is your feature request related to a problem? Please describe.

I frequently wish to combine force fields, like Sage with a different water model. This works fine when loading from disk:

sage_with_opc = ForceField("openff-2.2.1.offxml", "opc.offxml")

However, if I'm in the middle of a workflow and have already loaded them up into memory, there's no straightforward way to combine ForceFields.

Describe the solution you'd like

It'd be nice to be able to do something like

sage = SmallMoleculeForceFieldProvider.get("openff")
opc = WaterModelProvider.get("opc")
sage_with_opc = sage.combine(opc)

Parameter ordering, parameter section compatibility, etc. should follow the existing behavior.

Describe alternatives you've considered

I can handle this with an extra trip to disk, but this is slow and I'd argue should not be necessary.

sage.to_file("file1.offxml")
opc.to_file("file2.offxml")
sage_with_opc = ForceField("file1.offxml", "file2.offxml")

Additional context

Combining force fields has behavior consequences which are potentially significant and often result in worse performance. Combining force fields, however, is part of the specification and currently supported in de-serialization, so I don't think it's reasonable to claim this behavior out to be unsupported.
@j-wags
Copy link
Member

j-wags commented Oct 22, 2024

Agree that this would be great, and with your specific proposal to add a combine method. I'd be happy to take a PR for this, and we could probably refactor the ForceField initializer to use this code path when multiple sources are given as input as well.

@mattwthompson
Copy link
Member Author

How are identical parameters meant to be handled? There is existing discussion on parameters with duplicate SMIRKS patterns (#1363) but I'm not sure the reasons for allowing duplicate SMIRKS patterns extend to duplicate parameters.

Here is the shortest demonstration:

In [1]: from openff.toolkit import ForceField

In [2]: ForceField("tip3p.offxml", "tip3p.offxml")['Constraints'].parameters
/Users/mattthompson/micromamba/envs/openff-toolkit-test/lib/python3.12/site-packages/smirnoff99frosst/smirnoff99frosst.py:11: UserWarning: Module openff was already imported from None, but /Users/mattthompson/software/openff-toolkit is being added to sys.path
  from pkg_resources import resource_filename
Out[2]:
[<ConstraintType with smirks: [#1:1]-[#8X2H2+0:2]-[#1]  id: c-tip3p-H-O  distance: 0.9572 angstrom  >,
 <ConstraintType with smirks: [#1:1]-[#8X2H2+0]-[#1:2]  id: c-tip3p-H-O-H  distance: 1.5139006545247014 angstrom  >,
 <ConstraintType with smirks: [#1:1]-[#8X2H2+0:2]-[#1]  id: c-tip3p-H-O  distance: 0.9572 angstrom  >,
 <ConstraintType with smirks: [#1:1]-[#8X2H2+0]-[#1:2]  id: c-tip3p-H-O-H  distance: 1.5139006545247014 angstrom  >]

@mattwthompson mattwthompson linked a pull request Jan 14, 2025 that will close this issue
Open
5 tasks
@j-wags
Copy link
Member

j-wags commented Jan 15, 2025

Similar to #1363, I'd like to continue allowing duplicate SMIRKS. It's a weak preference, but given that I don't want to make any guarantees about policing the state of ParameterLists (that is, checking for duplicate SMIRKS upon modification) and I want to remain agnostic to user intent (since by the time people are modifying FFs, they're more advanced users), I think it's best to have this new API point behave naively and not check for duplicates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
reliability
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add ForceField.combine openforcefield/openff-toolkit
2 participants
@mattwthompson @j-wags