Non-deterministic charge validation by jthorton · Pull Request #2132 · OpenFreeEnergy/openfe

jthorton · 2026-08-11T11:35:11Z

Fixes #2115 by adding a validation method that attempts to catch partial charge generation methods which are used at runtime and are nondeterministic.

LLM / AI generated code disclosure

LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: yes / no
If yes, please provide details here: No

Checklist

All new code is appropriately documented (user-facing code must have complete docstrings).
Added a news entry, or the changes are not user-facing.
Ran pre-commit: you can run pre-commit locally or comment on this PR with pre-commit.ci autofix.
Filled in the AI generated code disclosure.

Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).

GPU integration tests
example notebook testing
packaging tests: run this for any large feature PRs or PRs that add test data.

Developers certificate of origin

I certify that this contribution is covered by the MIT License here and the Developer Certificate of Origin at https://developercertificate.org/.

jthorton · 2026-08-11T11:35:33Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

codecov · 2026-08-11T12:10:08Z

Codecov Report

❌ Patch coverage is 97.60000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.50%. Comparing base (110503a) to head (9cb4322).

Files with missing lines	Patch %	Lines
...openfe/protocols/openmm_utils/system_validation.py	87.50%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2132      +/-   ##
==========================================
- Coverage   95.20%   90.50%   -4.70%     
==========================================
  Files         213      213              
  Lines       21138    21146       +8     
==========================================
- Hits        20124    19139     -985     
- Misses       1014     2007     +993

Flag	Coverage Δ
fast-tests	`90.50% <97.60%> (?)`
slow-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

hannahbaumann · 2026-08-13T11:44:35Z

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


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?

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?

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

hannahbaumann · 2026-08-13T11:44:44Z

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


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

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

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?

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.

IAlibay

I think this matches what we discussed yesterday - as you mentioned, we should be able to drop the partial charge assignment calls in Protocols with this.

IAlibay · 2026-08-13T14:03:58Z

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


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

# Conflicts: # src/openfe/tests/protocols/test_openmmutils.py

jthorton · 2026-08-17T10:57:15Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

jthorton · 2026-08-17T13:17:27Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

github-actions · 2026-08-17T13:18:13Z

🚨 API breaking changes detected! 🚨

View workflow run

Griffe output

$ griffe check "openfe" -s src --no-inspection --no-color --verbose -a origin/main
src/openfe/tests/conftest.py:381: CN_molecule:
Return types are incompatible:
  Old: list[SmallMoleculeComponent]
  New: None

src/openfe/tests/protocols/openmm_md/test_plain_md_protocol.py:0: test_dry_run_charge_backends:
Public object was removed

src/openfe/tests/protocols/openmm_md/test_plain_md_protocol.py:0: BENZ:
Public object was removed

src/openfe/tests/protocols/openmm_md/test_plain_md_protocol.py:0: PYRIDINE:
Public object was removed

src/openfe/tests/protocols/openmm_ahfe/test_ahfe_protocol.py:0: test_dry_run_charge_backends:
Public object was removed

src/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py:414: BENZ:
Attribute value was changed:
  Old: 'benzene\n  PyMOL2.5          3D                             0\n\n 12 12  0  0  0  0  0  0  0  0999 V2000\n    1.4045   -0.0000    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n    0.7022    1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n   -0.7023    1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n   -1.4045   -0.0000    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n   -0.7023   -1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n    0.7023   -1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n    2.5079   -0.0000    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n    1.2540    2.1720    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n   -1.2540    2.1720    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n   -2.5079   -0.0000    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n   -1.2540   -2.1719    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n    1.2540   -2.1720    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n  1  2  2  0  0  0  0\n  1  6  1  0  0  0  0\n  1  7  1  0  0  0  0\n  2  3  1  0  0  0  0\n  2  8  1  0  0  0  0\n  3  4  2  0  0  0  0\n  3  9  1  0  0  0  0\n  4  5  1  0  0  0  0\n  4 10  1  0  0  0  0\n  5  6  2  0  0  0  0\n  5 11  1  0  0  0  0\n  6 12  1  0  0  0  0\nM  END\n$$$$\n'
  New: 'benzene\n     RDKit          3D\n\n 12 12  0  0  0  0  0  0  0  0999 V2000\n    1.4045   -0.0000    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n    0.7022    1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n   -0.7023    1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n   -1.4045   -0.0000    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n   -0.7023   -1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n    0.7023   -1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n    2.5079   -0.0000    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n    1.2540    2.1720    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n   -1.2540    2.1720    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n   -2.5079   -0.0000    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n   -1.2540   -2.1719    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n    1.2540   -2.1720    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n  1  2  2  0\n  1  6  1  0\n  1  7  1  0\n  2  3  1  0\n  2  8  1  0\n  3  4  2  0\n  3  9  1  0\n  4  5  1  0\n  4 10  1  0\n  5  6  2  0\n  5 11  1  0\n  6 12  1  0\nM  END\n\n>  <ofe-name>\nbenzene\n\n>  <atom.dprop.PartialCharge>\n-0.13016000265876451 -0.13009999568263689 -0.13009999568263689 -0.13009999568263689 -0.13009999568263689 -0.13009999568263689 0.13010999684532484 0.13010999684532484 0.13010999684532484 0.13010999684532484 0.13010999684532484 0.13010999684532484\n\n$$$$\n'

src/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py:454: PYRIDINE:
Attribute value was changed:
  Old: 'pyridine\n  PyMOL2.5          3D                             0\n\n 11 11  0  0  0  0  0  0  0  0999 V2000\n    1.4045   -0.0000    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n   -0.7023    1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n   -1.4045   -0.0000    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n   -0.7023   -1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n    0.7023   -1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n    2.4940   -0.0325    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n    1.2473   -2.1604    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n   -1.2473   -2.1604    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n   -2.4945   -0.0000    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n   -1.2753    2.1437    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n    0.7525    1.3034    0.0000 N   0  0  0  0  0  0  0  0  0  0  0  0\n  1  5  1  0  0  0  0\n  1  6  1  0  0  0  0\n  1 11  2  0  0  0  0\n  2  3  2  0  0  0  0\n  2 10  1  0  0  0  0\n  3  4  1  0  0  0  0\n  3  9  1  0  0  0  0\n  4  5  2  0  0  0  0\n  4  8  1  0  0  0  0\n  5  7  1  0  0  0  0\n  2 11  1  0  0  0  0\nM  END\n$$$$\n'
  New: 'pyridine\n     RDKit          3D\n\n 11 11  0  0  0  0  0  0  0  0999 V2000\n    1.4045   -0.0000    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n   -0.7023    1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n   -1.4045   -0.0000    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n   -0.7023   -1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n    0.7023   -1.2164    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0\n    2.4940   -0.0325    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n    1.2473   -2.1604    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n   -1.2473   -2.1604    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n   -2.4945   -0.0000    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n   -1.2753    2.1437    0.0000 H   0  0  0  0  0  0  0  0  0  0  0  0\n    0.7525    1.3034    0.0000 N   0  0  0  0  0  0  0  0  0  0  0  0\n  1  5  2  0\n  1  6  1  0\n  1 11  1  0\n  2  3  1  0\n  2 10  1  0\n  3  4  2  0\n  3  9  1  0\n  4  5  1  0\n  4  8  1  0\n  5  7  1  0\n  2 11  2  0\nM  END\n\n>  <atom.dprop.PartialCharge>\n0.39228000661188905 0.39228000661188905 -0.24653000215237791 -0.093170007860118698 -0.24653000215237791 0.021279994229024105 0.14280999621207063 0.1373999955301935 0.14280999621207063\n0.021279994229024105 -0.66390997747128655\n\n$$$$\n'

src/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py:748: test_setup_charge_backends(tmp_path):
Parameter was removed

src/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py:748: test_setup_charge_backends(method):
Parameter was removed

src/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py:748: test_setup_charge_backends(backend):
Parameter was removed

src/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py:748: test_setup_charge_backends(ref_key):
Parameter was removed

src/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py:748: test_setup_charge_backends(vac_settings):
Positional parameter was moved
  Details: position: from 5 to 1 (-4)

src/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py:748: test_setup_charge_backends(am1bcc_ref_charges):
Parameter was removed


$ griffe check "openfecli" -s src --no-inspection --no-color --verbose -a origin/main

jthorton · 2026-08-18T13:17:57Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

jthorton · 2026-08-18T13:46:10Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

jthorton · 2026-08-18T13:47:45Z


-@pytest.mark.parametrize("num", [i for i in range(1, 30)])
-def test_plot_2D_rmsd(num):
+def test_plot_2D_rmsd():


Sneaking this in can remove if wanted, though, locally this goes from 8s to 650ms while testing more aspects of the output.

Please move it to a separate PR.

jthorton · 2026-08-19T12:18:53Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

jthorton · 2026-08-19T12:20:12Z

@@ -27,7 +27,7 @@ def test_get_molecule_sdf():


 def test_get_molecule_mol2():


Should we allow loading from mol2? If we go via rdkit -> SMC we silently drop the charges which the users might have intended to use?

For now yes, we should retain mol2 as an option - this is because it's something we used to offer and we shouldn't break the API over it.

If we're worried about dropping charges, then the best option for now would be to validate & warn at SMC creation.

Yeah I think we need to either keep them or warn as they also silently get replaced with am1bcc currently, gufe issue OpenFreeEnergy/gufe#828

IAlibay

First look through.

IAlibay · 2026-08-20T14:43:12Z

Are we moving these to mol2 because we want to use them elsewhere?

I have changed them over to SDF as the charges are getting dropped when reading from mol2, triggering the new validation error in tests, the other option is to update gufe to pull these charges when building from rdkit?

IAlibay · 2026-08-20T15:00:38Z


-@pytest.mark.parametrize("num", [i for i in range(1, 30)])
-def test_plot_2D_rmsd(num):
+def test_plot_2D_rmsd():


Please move it to a separate PR.

IAlibay · 2026-08-21T00:25:03Z

+
+        # We count library and nagl charges as deterministic
+        # While users could use a deterministic charge method with increments we don't currently support this
+        if not labels["LibraryCharges"] and "NAGLCharges" not in labels:


Is LibraryCharges always guaranteed as a key or could it KeyError?

Good catch it could be missing, added support for it and a test.

IAlibay · 2026-08-21T00:28:52Z

        settings = self._get_settings()

-        # Assign partial charges now to avoid any discrepancies later
-        self._assign_partial_charges(settings["charge_settings"], small_mols)


I've been thinking more about this one - we're silently removing this, but won't users get caught out if they do use a deterministic charge method in the partial charge settings, e.g. NAGL?

Yeah that would be the downside, users would have to either use a ff with a NAGL handler or pre-charge them. If we keep support for NAGL and espaloma charge methods at runtime only that could work with the default setting being forcefield. It could be confusing if users have to define the forcefield to use in two places 1) the charges setting object and 2) small molecule force field, and then what happens if these are different?

Ideally I would love to keep this as simple as possible for users, we already encourage charging before running so moving to openfe will do no runtime charge generation doesn't feel like too much of a change?

IAlibay · 2026-08-21T00:29:04Z

-def test_dry_run_charge_backends(
-    CN_molecule, tmp_path, method, backend, ref_key, protocol_dry_settings, am1bcc_ref_charges
-):
+def test_dry_run_missing_charges(CN_molecule, protocol_dry_settings):


It would be good to have a dry run where we use nagl charges

This would also raise the error, or do you mean with a forcefield with a NAGL handler?

IAlibay · 2026-08-21T00:30:02Z

 BENZ = """\
 benzene
-  PyMOL2.5          3D                             0
+     RDKit          3D


Why are these changing?

Ah this is me passing them through openff to get charges for them, they were being assigned during a test which now triggers the validation error, I can just add the partial charge tag and undo the other changes?

IAlibay · 2026-08-21T00:31:36Z

@@ -27,7 +27,7 @@ def test_get_molecule_sdf():


 def test_get_molecule_mol2():


For now yes, we should retain mol2 as an option - this is because it's something we used to offer and we shouldn't break the API over it.

IAlibay · 2026-08-21T00:32:16Z

@@ -27,7 +27,7 @@ def test_get_molecule_sdf():


 def test_get_molecule_mol2():


If we're worried about dropping charges, then the best option for now would be to validate & warn at SMC creation.

add charge validation

d7dcd14

[pre-commit.ci] auto fixes from pre-commit.com hooks

84174f3

for more information, see https://pre-commit.ci

add tests

3c76241

hannahbaumann reviewed Aug 13, 2026

View reviewed changes

IAlibay reviewed Aug 13, 2026

View reviewed changes

jthorton and others added 4 commits August 14, 2026 11:36

Merge branch 'main' into charge_validation

2874599

# Conflicts: # src/openfe/tests/protocols/test_openmmutils.py

add validation to all protocols, remove runtime charge generation

36784f9

fix tests

251474c

Merge branch 'main' into charge_validation

ba3f47d

pre-commit-ci Bot and others added 2 commits August 17, 2026 10:57

[pre-commit.ci] auto fixes from pre-commit.com hooks

045954d

for more information, see https://pre-commit.ci

fix protocol charge generation tests

6c0d7d4

[pre-commit.ci] auto fixes from pre-commit.com hooks

960b874

for more information, see https://pre-commit.ci

jthorton added 2 commits August 17, 2026 16:02

fix tests with missing charges

96b5d24

Merge branch 'main' into charge_validation

e2099ed

pre-commit-ci Bot and others added 3 commits August 18, 2026 13:18

[pre-commit.ci] auto fixes from pre-commit.com hooks

b8f7ecb

for more information, see https://pre-commit.ci

speedup smoke test

98152db

add mixed charge/uncharged system testing

df24ca1

[pre-commit.ci] auto fixes from pre-commit.com hooks

7dfdb3f

for more information, see https://pre-commit.ci

jthorton commented Aug 18, 2026

View reviewed changes

jthorton added 3 commits August 19, 2026 11:07

switch to using sdfs

0c8e7dd

remove extra file

540aefa

fix cli tests

bacb1c3

[pre-commit.ci] auto fixes from pre-commit.com hooks

9cb4322

for more information, see https://pre-commit.ci

jthorton commented Aug 19, 2026

View reviewed changes

Comment thread src/openfe/tests/protocols/test_openmmutils.py Outdated

jthorton mentioned this pull request Aug 19, 2026

OpenFE silently ignores SMIRNOFF LibraryCharges and applies AM1-BCC when molecule charges are absent #2062

Open

jthorton and others added 3 commits August 20, 2026 10:51

add charges to inputs, remove slow mark

7b7e4ee

Update src/openfe/tests/protocols/test_openmmutils.py

c43dbf0

add news, update error to list all molecules with missing charges.

6c32c99

IAlibay self-requested a review August 20, 2026 14:08

IAlibay requested changes Aug 21, 2026

View reviewed changes

catch missing librarycharges handler in ff

4cf822d

		@@ -27,7 +27,7 @@ def test_get_molecule_sdf():


		def test_get_molecule_mol2():

Uh oh!

Conversation

jthorton commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LLM / AI generated code disclosure

Developers certificate of origin

Uh oh!

jthorton commented Aug 11, 2026

Uh oh!

codecov Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jthorton commented Aug 17, 2026

Uh oh!

jthorton commented Aug 17, 2026

Uh oh!

github-actions Bot commented Aug 17, 2026

Uh oh!

jthorton commented Aug 18, 2026

Uh oh!

jthorton commented Aug 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jthorton commented Aug 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jthorton Aug 21, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jthorton commented Aug 11, 2026 •

edited

Loading

codecov Bot commented Aug 11, 2026 •

edited

Loading

jthorton Aug 21, 2026 •

edited

Loading