-
Notifications
You must be signed in to change notification settings - Fork 107
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.
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
atomate2 / OpenMM OPLS-AA Enhancements #1111
base: main
Are you sure you want to change the base?
Changes from 15 commits
65f68a7
d50a597
925e117
9864f8d
05ae123
91a1c88
b042cae
5226c75
84baad8
9883fe7
fc01e94
ad4ff11
b45af33
36fd518
3ba99ee
86d1102
979f3f0
ea70919
e195cdc
0a7e920
40fbe0b
90d326c
ede6f44
eedcb68
e9cf207
02b15b8
cd9b200
1192345
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,25 +5,29 @@ | |
import copy | ||
import io | ||
import re | ||
import textwrap | ||
import warnings | ||
import xml.etree.ElementTree as ET | ||
from pathlib import Path | ||
from xml.etree.ElementTree import tostring | ||
|
||
import defusedxml.ElementTree as DiffET | ||
import numpy as np | ||
from emmet.core.openff import MoleculeSpec | ||
from emmet.core.openmm import OpenMMInterchange, OpenMMTaskDocument | ||
from emmet.core.vasp.task_valid import TaskState | ||
from jobflow import Response | ||
from openmm import Context, LangevinMiddleIntegrator, System, XmlSerializer | ||
from openmm.app import PME, ForceField | ||
from openmm.app.forcefield import NonbondedGenerator | ||
from openmm.app.pdbfile import PDBFile | ||
from openmm.unit import kelvin, picoseconds | ||
from pymatgen.core import Element | ||
from pymatgen.io.openff import get_atom_map | ||
|
||
from atomate2.openff.utils import create_mol_spec, merge_specs_by_name_and_smiles | ||
from atomate2.openmm.jobs.base import openmm_job | ||
from atomate2.openmm.utils import opls_lj | ||
|
||
try: | ||
import openff.toolkit as tk | ||
|
@@ -40,7 +44,10 @@ class XMLMoleculeFF: | |
|
||
def __init__(self, xml_string: str) -> None: | ||
"""Create an XMLMoleculeFF object from a string version of the XML file.""" | ||
self.tree = ET.parse(io.StringIO(xml_string)) # noqa: S314 | ||
try: | ||
self.tree = ET.parse(io.StringIO(xml_string)) # noqa: S314 | ||
except ET.ParseError: | ||
self.tree = DiffET.parse(xml_string) | ||
|
||
root = self.tree.getroot() | ||
canonical_order = {} | ||
|
@@ -153,11 +160,10 @@ def from_file(cls, file: str | Path) -> XMLMoleculeFF: | |
return cls(xml_str) | ||
|
||
|
||
def create_system_from_xml( | ||
topology: tk.Topology, | ||
def create_ff_from_xml( | ||
xml_mols: list[XMLMoleculeFF], | ||
) -> System: | ||
"""Create an OpenMM system from a list of molecule specifications and XML files.""" | ||
"""Create OpenMM forcefield from a list of molecule specifications and XML files.""" | ||
io_files = [] | ||
for i, xml in enumerate(xml_mols): | ||
xml_copy = copy.deepcopy(xml) | ||
|
@@ -168,7 +174,7 @@ def create_system_from_xml( | |
for i, xml in enumerate(io_files[1:]): # type: ignore[assignment] | ||
ff.loadFile(xml, resname_prefix=f"_{i + 1}") | ||
|
||
return ff.createSystem(topology.to_openmm(), nonbondedMethod=PME) | ||
return ff | ||
|
||
|
||
@openmm_job | ||
|
@@ -280,7 +286,26 @@ def generate_openmm_interchange( | |
**pack_box_kwargs, | ||
) | ||
|
||
system = create_system_from_xml(topology, xml_mols) | ||
ff = create_ff_from_xml(xml_mols) | ||
|
||
# obtain 14 scaling values from forcefield | ||
generator = ff.getGenerators() | ||
for gen in generator: | ||
if isinstance(gen, NonbondedGenerator): | ||
c14 = gen.coulomb14scale | ||
lj14 = gen.lj14scale | ||
|
||
system = ff.createSystem(topology.to_openmm(), nonbondedMethod=PME) | ||
if "opls" in tags: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tags should just be metadata, not core logic. You can add another mixing related kwarg as needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point--I've replaced this behavior with an |
||
if (c14 != 0.5) or (lj14 != 0.5): | ||
raise ValueError( | ||
textwrap.dedent(f"""\ | ||
NonbondedForce class in XML, | ||
<NonbondedForce coulomb14scale="0.5" lj14scale="0.5">, | ||
does not match OPLS convention, | ||
<NonbondedForce coulomb14scale="{c14}" lj14scale="{lj14}">.""") | ||
) | ||
system = opls_lj(system) | ||
|
||
# these values don't actually matter because integrator is only | ||
# used to generate the state | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need defusedxml in particular? Best to avoid adding a dependency if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ruff gave me the following error for
ET.parse
,I now see that
# noqa: S314
was there in the original version to suppress this error, so I've updated the try/except statement, now without defusedxml. I also removed the textwrap dependency.