atomate2 / OpenMM OPLS-AA Enhancements #1111

shehan807 · 2025-01-25T18:26:47Z

Summary

This PR resolves the first two enhancements proposed in Issue #1102:

Feature 1: Implements the correct geometric combination rules for OPLS MD simulations via opls_lj()

I've changed create_system_from_xml to create_ff_from_xml in src/atomate2/openmm/jobs/generate.py. Why? The OPLS geometric combination rules have two requirements, (1) a CustomNonbondedForce in OpenMM that combines the sigma and eps parameters geometrically, and (2) the coulomb14scale and lj14scale attributes of the ForceField are both 0.5. The latter was enough justification for raising an error if not the case--to do so, the ForceField object would need to be accessible in generate_openmm_interchange (instead of adding unsuitable arguments to create_system_from_xml). Originally, create_system_from_xml created both the ForceField and System within the same function; now, system and ff are separate variables in generate_openmm_interchange.

Feature 2: Allows users to create OPLS forcefield files via generate_opls_xml() via LigParGen and BOSS source code directly. To do so, users would need to follow the steps in the docs from this fork.

Currently, generate_opls_xml is only functional so long as the user has a local installation of Docker and created a private (~~for now, pending approval for pushing to public registries~~ approval received to save on public registries) image of LigParGen. The documentation is provided to completion in docs/user/codes/openmm.md.

EDIT: We've received written approval from Dr. William Jorgensen to host the LigParGen image on a public registry, i.e., the user no longer has to build this themself.

Feature 3: All changes from Generalize download_opls_xml Function #1054 have been merged into this fork (i.e., extending the download_opls_xml to handle charges, number of optimization iterations, and charge method).

Additional dependencies introduced

~~defusedxml, resolves ruff failures for safely loading XML files, e.g.,~~
~~textwrap, enables easier text wrapping of long warnings/errors printed for the user, e.g.,~~

Checklist

Code is in the standard Python style.
The easiest way to handle this is to run the following in the correct sequence on
your local machine. Start with running ruff and ruff format on your new code. This will
automatically reformat your code to PEP8 conventions and fix many linting issues.
Doc strings have been added in the Numpy docstring format.
Run ruff on your code.
Type annotations are highly encouraged. Run mypy to
type check your code.
Tests have been added for any new functionality or bug fixes.
All linting and tests pass.

CC'ing @utf, @orionarcher

…haps this is fine? and (2) usused eps14 variable (need to check that eps == eps14)

…tead; solution: comment out eps14 line to match original ligpargen code

… 14 interaction scale values (i.e., 0.5); changed create_system_from_xml to create_ff_from_xml to enable generate_openmm_interchange to have access to combination rules only available via the forcefield...still experiencing NoneType error when creating OpenMMInterchange object which prohibits inspection of OpenMMInterchange in generate_openmm_interchange

… to convert xml to string, leaving this commented for now before checking other cases

…to cutoff; incorporated diffusedxml to follow linter guidelines

…ges missing

… and solution was to .split() command into a list of strings

…rate_opls_xml(...) is complete

… generate OPLS .xml files

orionarcher

Couple comments but generally looks good!

orionarcher · 2025-03-06T01:29:06Z

src/atomate2/openmm/jobs/generate.py

 from pathlib import Path
 from xml.etree.ElementTree import tostring

+import defusedxml.ElementTree as DiffET


Do we need defusedxml in particular? Best to avoid adding a dependency if possible

ruff gave me the following error for ET.parse,

S314 Using `xml` to parse untrusted data is known to be vulnerable to XML attacks; use `defusedxml` equivalents

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.

orionarcher · 2025-03-06T01:35:08Z

src/atomate2/openmm/jobs/generate.py

+            lj14 = gen.lj14scale
+
+    system = ff.createSystem(topology.to_openmm(), nonbondedMethod=PME)
+    if "opls" in tags:


tags should just be metadata, not core logic. You can add another mixing related kwarg as needed

Good point--I've replaced this behavior with an ff_kwargs argument to generate_openmm_interchange.

orionarcher · 2025-03-06T01:37:01Z

src/atomate2/openmm/utils.py

-    names_smiles: dict[str, str], output_dir: str | Path, overwrite_files: bool = False
+    names_params: dict[str, dict[str, str]],


can you document what params are allowed?

orionarcher · 2025-03-06T01:38:41Z

tests/openmm_md/jobs/test_generate.py



-def test_create_system_from_xml(openmm_data):
+def test_ff_system_from_xml(openmm_data):


-> test_create_ff_from_xml

orionarcher · 2025-03-06T01:40:22Z

@janosh or @utf could you get the tests running here?

shehan807 · 2025-03-08T20:05:44Z

@janosh all pre-commit/pytests are working on my end as of my last commit, could you try running the tests again? Thanks @orionarcher for the comments!

shehan807 · 2025-03-10T18:10:38Z

@janosh The last failed test is due to an environment variable dependence introduced by the generate_opls_xml function. The function requires the user to follow the OPLS docs to create a CONTAINER_SOFTWARE and LPG_IMAGE_NAME environment variable:

export LPG_IMAGE_NAME="USERNAME/ligpargen:latest"
export CONTAINER_SOFTWARE="podman-hpc" # e.g.

Based on what I've observed from the VASP/AIMS tests, I just updated test_generate_opls_xml so it monkeypatches environment variables. The new tests are now broken up into test_generate_opls_xml_no_container and test_generate_opls_xml_success. The latter is skipped since, even if the env variables were set, it would fail because success depends on whether podman-hpc or docker are available.

merge up to 3/10

orionarcher · 2025-03-14T14:26:16Z

@janosh or @utf the tests are frozen again here, could we get them running here?

Sorry to bother, I'm not sure why another approval is required.

orionarcher

Thanks for addressing my comments, this looks good on my end!

janosh

looks good at a cursory glance (not my wheelhouse 😄)

* added opls_lj function; pre-commit fails for (1) eps._value call, perhaps this is fine? and (2) usused eps14 variable (need to check that eps == eps14) * eps14 is not the correct value, off by 0.5 and eps should be used instead; solution: comment out eps14 line to match original ligpargen code * initial commit to implementing opls_lj with error checking for proper 14 interaction scale values (i.e., 0.5); changed create_system_from_xml to create_ff_from_xml to enable generate_openmm_interchange to have access to combination rules only available via the forcefield...still experiencing NoneType error when creating OpenMMInterchange object which prohibits inspection of OpenMMInterchange in generate_openmm_interchange * the use of io.StringIO is seemingly a bug? works fine without needing to convert xml to string, leaving this commented for now before checking other cases * added try/except to handle io.StringIO failure * added 1-4 scaling conditional statement; fized opls nonbonded method to cutoff; incorporated diffusedxml to follow linter guidelines * try/except failure should specify selenium or webdriver_manager packages missing * added generate_opls_xml function with placeholder for ligpargen commands * fixed shutil.copy to shutil.move to avoid redundant files in tmpdir * fixed linter issues (namely, subprocess.run(...shell=True) was unsafe and solution was to .split() command into a list of strings * completed OPLS Docker documentation + except for shifter issues, generate_opls_xml(...) is complete * completed boss->ligpargen->docker->podman-hpc->subprocess pipeline to generate OPLS .xml files * minor typo in docs * updated tests for download_ and generate_opls_xml functions * removed defusedxml dependency * removed textwrap dependency * replaced tag-based opls flag to ff_kwargs for opls * added names_params documentation * test_create_ff_from_xml * pre-commit * ff_kwargs is not None * all new tests pass pytest * added monkeypatching env variables * fixed docker and/or podman-hpc edge case * removed rogue file

Shehan M Parmar and others added 14 commits January 16, 2025 14:27

added opls_lj function; pre-commit fails for (1) eps._value call, per…

65f68a7

…haps this is fine? and (2) usused eps14 variable (need to check that eps == eps14)

eps14 is not the correct value, off by 0.5 and eps should be used ins…

d50a597

…tead; solution: comment out eps14 line to match original ligpargen code

the use of io.StringIO is seemingly a bug? works fine without needing…

9864f8d

… to convert xml to string, leaving this commented for now before checking other cases

added try/except to handle io.StringIO failure

05ae123

added 1-4 scaling conditional statement; fized opls nonbonded method …

91a1c88

…to cutoff; incorporated diffusedxml to follow linter guidelines

try/except failure should specify selenium or webdriver_manager packa…

b042cae

…ges missing

added generate_opls_xml function with placeholder for ligpargen commands

5226c75

fixed shutil.copy to shutil.move to avoid redundant files in tmpdir

84baad8

fixed linter issues (namely, subprocess.run(...shell=True) was unsafe…

9883fe7

… and solution was to .split() command into a list of strings

completed OPLS Docker documentation + except for shifter issues, gene…

fc01e94

…rate_opls_xml(...) is complete

Merge branch 'materialsproject:main' into feature/OPLS-AA

ad4ff11

completed boss->ligpargen->docker->podman-hpc->subprocess pipeline to…

b45af33

… generate OPLS .xml files

minor typo in docs

36fd518

shehan807 mentioned this pull request Jan 25, 2025

Generalize download_opls_xml Function #1054

Closed

5 tasks

updated tests for download_ and generate_opls_xml functions

3ba99ee

shehan807 mentioned this pull request Feb 11, 2025

atomate2 / OpenMM Enhancements: Added OPLS Support #1102

Closed

orionarcher approved these changes Mar 6, 2025

View reviewed changes

Shehan M Parmar and others added 9 commits March 6, 2025 08:44

removed defusedxml dependency

86d1102

removed textwrap dependency

979f3f0

replaced tag-based opls flag to ff_kwargs for opls

ea70919

added names_params documentation

e195cdc

test_create_ff_from_xml

0a7e920

pre-commit

40fbe0b

ff_kwargs is not None

90d326c

all new tests pass pytest

ede6f44

final pre-commit

eedcb68

shehan807 requested a review from orionarcher March 8, 2025 20:04

added monkeypatching env variables

e9cf207

Shehan M Parmar and others added 3 commits March 10, 2025 12:37

fixed docker and/or podman-hpc edge case

02b15b8

removed rogue file

cd9b200

Merge pull request #2 from materialsproject/main

1192345

merge up to 3/10

orionarcher approved these changes Mar 14, 2025

View reviewed changes

janosh approved these changes Mar 14, 2025

View reviewed changes

shehan807 mentioned this pull request Mar 19, 2025

Add DynamicOpenMMFlowMaker for dynamic OpenMM Simulations #1115

Merged

5 tasks

janosh merged commit 26deeb8 into materialsproject:main Mar 28, 2025
18 checks passed

		names_smiles: dict[str, str], output_dir: str \| Path, overwrite_files: bool = False
		names_params: dict[str, dict[str, str]],



		def test_create_system_from_xml(openmm_data):
		def test_ff_system_from_xml(openmm_data):

atomate2 / OpenMM OPLS-AA Enhancements #1111

atomate2 / OpenMM OPLS-AA Enhancements #1111

Uh oh!

Conversation

shehan807 commented Jan 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Additional dependencies introduced

Checklist

Uh oh!

orionarcher left a comment

Choose a reason for hiding this comment

Uh oh!

orionarcher Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

shehan807 Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orionarcher Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

shehan807 Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

orionarcher Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

shehan807 Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

orionarcher Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

orionarcher commented Mar 6, 2025

Uh oh!

shehan807 commented Mar 8, 2025

Uh oh!

shehan807 commented Mar 10, 2025

Uh oh!

orionarcher commented Mar 14, 2025

Uh oh!

orionarcher left a comment

Choose a reason for hiding this comment

Uh oh!

janosh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shehan807 commented Jan 25, 2025 •

edited

Loading

shehan807 Mar 6, 2025 •

edited

Loading