Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
d7dcd14
add charge validation
jthorton Aug 11, 2026
84174f3
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 11, 2026
3c76241
add tests
jthorton Aug 11, 2026
2874599
Merge branch 'main' into charge_validation
jthorton Aug 14, 2026
36784f9
add validation to all protocols, remove runtime charge generation
jthorton Aug 14, 2026
251474c
fix tests
jthorton Aug 17, 2026
ba3f47d
Merge branch 'main' into charge_validation
jthorton Aug 17, 2026
045954d
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 17, 2026
6c0d7d4
fix protocol charge generation tests
jthorton Aug 17, 2026
960b874
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 17, 2026
96b5d24
fix tests with missing charges
jthorton Aug 17, 2026
e2099ed
Merge branch 'main' into charge_validation
jthorton Aug 18, 2026
b8f7ecb
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 18, 2026
98152db
speedup smoke test
jthorton Aug 18, 2026
df24ca1
add mixed charge/uncharged system testing
jthorton Aug 18, 2026
7dfdb3f
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 18, 2026
0c8e7dd
switch to using sdfs
jthorton Aug 19, 2026
540aefa
remove extra file
jthorton Aug 19, 2026
bacb1c3
fix cli tests
jthorton Aug 19, 2026
9cb4322
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 19, 2026
7b7e4ee
add charges to inputs, remove slow mark
jthorton Aug 20, 2026
c43dbf0
Update src/openfe/tests/protocols/test_openmmutils.py
jthorton Aug 20, 2026
6c32c99
add news, update error to list all molecules with missing charges.
jthorton Aug 20, 2026
4cf822d
catch missing librarycharges handler in ff
jthorton Aug 21, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/openfe/protocols/openmm_utils/system_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
SolventComponent,
)
from gufe.components.errors import ComponentValidationError
from gufe.protocols.errors import ProtocolValidationError
from openff.toolkit import ForceField
from openff.toolkit import Molecule as OFFMol

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -371,3 +373,50 @@ def validate_chemical_system(system: ChemicalSystem):
except ComponentValidationError as e:
errmsg = f"Component {entry} from ChemicalSystem {system.name} failed validation: {e}"
raise ComponentValidationError(errmsg)


def validate_nondeterministic_charges(system: ChemicalSystem, small_molecule_forcefield: str):
"""
Validate that the SmallMoleculeComponents of the system will have deterministic partial charges.

This is determined by checking for charges on the molecules before checking what would be assigned by the force field.

Parameters
----------
system : ChemicalSystem
The ChemicalSystem to validate with SmallMoleculeComponents.
small_molecule_forcefield : str
The force field to be used for the SmallMoleculeComponents.

Raises
------
ProtocolValidationError
If any SmallMoleculeComponents in the system would have am1bcc charges generated at runtime.
"""
smcs: list[SmallMoleculeComponent] = system.get_components_of_type(SmallMoleculeComponent)
if "espaloma" in small_molecule_forcefield or "gaff" in small_molecule_forcefield:
# this will always generate charges at runtime, so raise an error for missing charges
ff = None
else:
# We do not check for offxml in the name as users can pass the force field contents as a string
ff = ForceField(small_molecule_forcefield)

for smc in smcs:
offmol = smc.to_openff()
if offmol.partial_charges is not None and np.any(offmol.partial_charges):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we want to support a case where the user supplies partial charges will all zeros?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 - given we have our own benchmark case where we want to do this, we probably should support it

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes good idea. Currently, this only works as a Library charge would we want to keep that as a source of protection to make sure users know what they are doing or allow it as charges on the molecule as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point - I think if it's easily allowed by LibraryCharge then it might be better to keep the check in. There's too many ways someone could accidentally add partial charges.

continue

# check the labels assigned for an openff force field
if ff is not None:
labels = ff.label_molecules(offmol.to_topology())[0]
else:
# return a dummy label as the gaff and espaloma should always give am1bcc charges
labels = {"LibraryCharges": {}}

# We count library and nagl charges as deterministic
if len(labels["LibraryCharges"]) != offmol.n_atoms and "NAGLCharges" not in labels:
errmsg = (
f"{smc} from system {system.name} would have am1bcc charges generated at runtime which is non-deterministic. "
f"Please provide a molecule with pre-computed charges or use library charges instead."
)
raise ProtocolValidationError(errmsg)
77 changes: 77 additions & 0 deletions src/openfe/tests/protocols/test_openmmutils.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,21 @@
import gzip
import logging
import os
import re
import sys
from importlib import resources
from pathlib import Path
from unittest import mock

import numpy as np
import pytest
from gufe import ChemicalSystem
from gufe.components.errors import ComponentValidationError
from gufe.protocols.errors import ProtocolValidationError
from gufe.settings import OpenMMSystemGeneratorFFSettings, ThermoSettings
from numpy.testing import assert_allclose, assert_equal
from openff.toolkit import Molecule as OFFMol
from openff.toolkit import ForceField
from openff.toolkit.utils.toolkit_registry import ToolkitRegistry
from openff.toolkit.utils.toolkits import RDKitToolkitWrapper
from openff.units import unit
Expand Down Expand Up @@ -1283,3 +1287,76 @@ def test_set_metadata_none_clears():
_set_offmol_metadata(mol, "residue_name", "LIG")
_set_offmol_metadata(mol, "residue_name", None)
assert all("residue_name" not in a.metadata for a in mol.atoms)


class TestChargeValidation:
"""Test validation of nondeterministic partial charge assignment."""

@pytest.fixture
def benzene_charged_system(self, benzene_modifications):
return ChemicalSystem({"ligand": benzene_modifications["benzene"]}, name="charged")

@pytest.fixture

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to also add a test where the first smc is charged and the second does not have charges? How would we want to handle such a case?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test with a system with a mix of charged and uncharged SMCs, this will raise an error on the first one that fails validation. Do you think it would be better however to list all of the SMCs which fail to avoid playing wack-a-mole?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to flag all components in the ChemicalSystem which fail validation in one error message to hopefully make the experience better!

def benzene_no_charge_system(self, benzene_modifications_uncharged):
return ChemicalSystem({"ligand": benzene_modifications_uncharged["benzene"]}, name="no charges")

def test_gaff_with_molecule_charges(self, benzene_charged_system):
system_validation.validate_nondeterministic_charges(
benzene_charged_system,
small_molecule_forcefield="gaff-2.11"
)

def test_gaff_no_charges(self, benzene_no_charge_system):
with pytest.raises(ProtocolValidationError, match=re.escape("SmallMoleculeComponent(name=benzene) from system no charges would have am1bcc charges")):
system_validation.validate_nondeterministic_charges(
benzene_no_charge_system,
small_molecule_forcefield="gaff-2.11"
)

def test_espaloma_with_molecule_charges(self, benzene_charged_system):
system_validation.validate_nondeterministic_charges(
benzene_charged_system,
small_molecule_forcefield="espaloma-0.3.2"
)

def test_espaloma_no_charges(self, benzene_no_charge_system):
with pytest.raises(ProtocolValidationError, match=re.escape("SmallMoleculeComponent(name=benzene) from system no charges would have am1bcc charges")):
system_validation.validate_nondeterministic_charges(
benzene_no_charge_system,
small_molecule_forcefield="espaloma-0.3.2"
)

def test_openff_with_molecule_charges(self, benzene_charged_system):
system_validation.validate_nondeterministic_charges(
benzene_charged_system,
small_molecule_forcefield="openff-2.2.0.offxml"
)

def test_openff_no_charges(self, benzene_no_charge_system):
with pytest.raises(ProtocolValidationError, match=re.escape("SmallMoleculeComponent(name=benzene) from system no charges would have am1bcc charges")):
system_validation.validate_nondeterministic_charges(
benzene_no_charge_system,
small_molecule_forcefield="openff-2.2.0.offxml"
)

def test_openff_nagl_no_charges(self, benzene_no_charge_system):
system_validation.validate_nondeterministic_charges(
benzene_no_charge_system,
small_molecule_forcefield="openff-2.3.0.offxml"
)

def test_openff_lib_charges_no_charges(self, benzene_no_charge_system, benzene_modifications):
# add some library charges to the force field and test using a string
benzene = benzene_modifications["benzene"].to_openff()
# use a force field with an am1bcc handler
ff = ForceField("openff-2.2.0.offxml")
lib_charge_handler = ff.get_parameter_handler("LibraryCharges")
# make a new parameter
lib_charge = lib_charge_handler._INFOTYPE.from_molecule(benzene)
# add it to the handler
lib_charge_handler.add_parameter(parameter=lib_charge)
# run the validation
system_validation.validate_nondeterministic_charges(
benzene_no_charge_system,
small_molecule_forcefield=ff.to_string()
)
Loading