Let the force field resolve the partial charge assignment by jthorton · Pull Request #2152 · OpenFreeEnergy/openfe

jthorton · 2026-08-12T10:33:52Z

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/.

github-actions · 2026-08-12T10:34:25Z

No API break detected ✅

View workflow run

Griffe output

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

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

codecov · 2026-08-12T11:04:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.33%. Comparing base (d1785c6) to head (d6cd80a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2152      +/-   ##
==========================================
- Coverage   95.04%   90.33%   -4.71%     
==========================================
  Files         206      206              
  Lines       20553    20660     +107     
==========================================
- Hits        19534    18664     -870     
- Misses       1019     1996     +977

Flag	Coverage Δ
fast-tests	`90.33% <100.00%> (?)`
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.

IAlibay

quick first look

jthorton · 2026-08-19T16:56:22Z

pre-commit.ci autofix

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

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

IAlibay

Couple of things - mainly it doesn't look like the CLI would work right now (maybe it just needs a test to check it).

IAlibay · 2026-08-20T14:26:27Z

+        # which is different to how settings work which can leave off the .offxml extension
+        ff = ForceField(*forcefields)
+        # let the force field resolve the partial charge assignment
+        charges = ff.get_partial_charges(offmol)


I believe this needs to get wrapped around toolkit_registry_manager, otherwise we'll go back to encountering the annoying rdkit & openeye toolkit aren't compatible problem.

Probably but which registry would we use as it could influence the charge method used? I think the default is openeye and am1bccelf10 and then fall back to AmberToolsam1bcc?

Just use what the user has in the settings for the toolkit_backend and make it clear in the docs that the backend is always followed when a charge is generated.

Ah this doesn't work if the force field uses nagl charges, as our default is ambertools, we might need to use a different method to make the registry. Maybe something like:
If the user has openeye pass:

openeye

nagl

ambertools
If the user has rdkit and no openeye:

rdkit

nagl

ambertools

Sorry I don't understand why it's not working.

The "AmberTools" backend is AmberTools + RDKit, that should be enough for NAGL to work no?

Ah ok - the issue is that the NAGL registry isn't in there?

I think it might be ok to just add the NAGLToolkitWrapper to both AmberTools & OpenEye backend lists - please double check but I think it will still do the "protection" that we're trying to do (i.e. it will block you from doing am1bcc with openeye if you don't want it).

Add the wrapper to all backends if nagl is available which I think is what we want?

jthorton · 2026-08-21T10:54:19Z

pre-commit.ci autofix

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

IAlibay

Overall looks good, but yeah we might need to change the backends behaviour. Let me know if you want to have a chat tomorow morning.

IAlibay · 2026-08-26T17:34:44Z

            off_toolkit_backend: ambertools
            number_of_conformers: None
            nagl_model: None
+            forcefields: None


How about including an example of this under the settings help section?

That section is already getting quite big, I think it would be better to point to the docs and have small examples there that users can copy for some different options, this would simplify the CLI help message as well!

Added the new options to the CLI settings template here

IAlibay · 2026-08-26T17:36:03Z

+    settings = {
+        "partial_charge": {
+            "method": "forcefield",
+            "settings": {"forcefields": ["openff_unconstrained-2.3.0"]},


[nit] Any reason for using unconstrained here? Might be better to use the default we use day-to-day.

No was just trying different one, will change to the normall one.

IAlibay · 2026-08-29T13:36:24Z

        if not overwrite:
            return offmol

+    if method.lower() == "forcefield":


Can we have a check for the other way around too? I'm thinking new users might not easily know you need to set both - especially via the CLI.

Add the reverse check and test.

… validation

jthorton · 2026-09-02T12:00:03Z

pre-commit.ci autofix

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

IAlibay · 2026-09-08T12:18:50Z

      Only ``ambertools`` and ``rdkit`` `off_toolkit_backend`` options
      are supported. A maximum of one conformer is allowed.

+    ``forcefield``:


I've been thinking about the impact of doing this - i.e. what would this mean to users if we merged this PR with a forcefield entry here and how confusing it would be at the Protocol level.

Is there any way we could not update these settings and just have it as a separate arg that gets picked up from the YAML file (i.e. I think right now we only need this for the CLI?).

I don't love it, but what if we a) created a subclass of settings specifically for the CLI, or b) just allowed extras?

I think this would depend on what we do with #2132, if we say that no charge assignment will be done by OpenFE and set the partial charge settings to None by default and have a warning that the option is deprecated and ignored at the protocol level then using this as the CLI charge settings class would be fine.

If not would we not want a to get the pydantic validation on the settings?

I think this PR has to try to exist standalone of any follow up. There's a good chance we won't be able to deal with the other PR properly ahead of the next release. It would be better if we could safely merge this PR first.

I'm going to advocate for a specific CLI settings subclass for now.

IAlibay · 2026-09-08T12:41:33Z

+                "via `forcefields`."
+            )
+            raise ValueError(errmsg)
+    elif forcefields is not None:


This can pass even if forcefields is an empty list,

Thanks this should be caught now!

jthorton · 2026-09-09T10:18:50Z

pre-commit.ci autofix

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

jthorton · 2026-09-14T13:51:01Z

pre-commit.ci autofix

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

IAlibay · 2026-09-14T14:09:26Z

From today's iteration meeting - we should add a test for a system with virtual sites.

IAlibay

Just the one thing, otherwise it looks good to me - approving early.

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

jthorton · 2026-09-16T12:18:49Z

pre-commit.ci autofix

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

IAlibay

lgtm, thanks!

try to get charges from the force field

cd183a8

jthorton commented Aug 12, 2026

View reviewed changes

Comment thread src/openfe/protocols/openmm_utils/charge_generation.py Outdated

IAlibay requested changes Aug 17, 2026

View reviewed changes

Merge branch 'main' into charge_from_ff

d1ecb40

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 added 2 commits August 19, 2026 17:53

add forcefield option for charge assignment

46c5bbf

remove import

f372a16

pre-commit-ci Bot and others added 3 commits August 19, 2026 16:56

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

7c0575e

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

fix rename, update error message

00692ec

Merge remote-tracking branch 'origin/charge_from_ff' into charge_from_ff

ebf4e78

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

jthorton requested a review from IAlibay August 20, 2026 08:32

update cli option, add readme

3eb3600

IAlibay requested changes Aug 20, 2026

View reviewed changes

finish CLI support and test, file extension handling

d700702

pre-commit-ci Bot and others added 5 commits August 21, 2026 10:54

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

5083dd0

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

fix whitespace

38ff7c6

Merge branch 'main' into charge_from_ff

5fbd1ce

use the toolkits in the input settings

aec1d35

fix whitespace

5616dc1

jthorton requested a review from IAlibay August 26, 2026 09:11

IAlibay reviewed Aug 26, 2026

View reviewed changes

jthorton mentioned this pull request Aug 28, 2026

Update the default samll_molecule_forcefield setting OpenFreeEnergy/gufe#831

Merged

3 tasks done

IAlibay reviewed Aug 29, 2026

View reviewed changes

jthorton and others added 2 commits September 2, 2026 12:58

Merge branch 'main' into charge_from_ff

b9a332f

PR feedback, update toolkit backends with nagl, add extra force field…

8c4421f

… validation

pre-commit-ci Bot and others added 3 commits September 2, 2026 12:00

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

76924a4

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

fix mypy

5610478

Merge branch 'main' into charge_from_ff

0e50422

jthorton requested a review from IAlibay September 7, 2026 14:03

jthorton mentioned this pull request Sep 7, 2026

Add new CLI charge methods example OpenFreeEnergy/ExampleNotebooks#325

Merged

IAlibay requested changes Sep 8, 2026

View reviewed changes

IAlibay reviewed Sep 8, 2026

View reviewed changes

jthorton and others added 2 commits September 9, 2026 11:04

Merge branch 'main' into charge_from_ff

6038475

catch forcefields is empty list

44102d9

pre-commit-ci Bot and others added 2 commits September 9, 2026 10:19

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

4cb249d

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

use new charge settings class for the cli

cb3fa7b

pre-commit-ci Bot and others added 4 commits September 14, 2026 13:51

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

617f07f

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

Merge branch 'main' into charge_from_ff

85af2be

remove whitespace

25d3928

Merge remote-tracking branch 'origin/charge_from_ff' into charge_from_ff

870f030

IAlibay approved these changes Sep 15, 2026

View reviewed changes

Comment thread src/openfe/protocols/openmm_utils/charge_generation.py Outdated

Comment thread src/openfe/protocols/openmm_utils/charge_generation.py Outdated

jthorton and others added 3 commits September 16, 2026 11:55

Update src/openfe/protocols/openmm_utils/charge_generation.py

998df08

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Update src/openfe/protocols/openmm_utils/charge_generation.py

289f674

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

support vsite force fields, fix mypy

9ef5b92

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

e53786f

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

atravitz added this to the 1.13.0 milestone Sep 16, 2026

IAlibay approved these changes Sep 17, 2026

View reviewed changes

Merge branch 'main' into charge_from_ff

d6cd80a

jthorton enabled auto-merge (squash) September 17, 2026 09:01

jthorton merged commit 6c163c1 into main Sep 17, 2026
9 checks passed

jthorton deleted the charge_from_ff branch September 17, 2026 09:05

Conversation

jthorton commented Aug 12, 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!

github-actions Bot commented Aug 12, 2026

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jthorton commented Aug 19, 2026

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jthorton commented Aug 21, 2026

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jthorton commented Sep 2, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jthorton commented Aug 12, 2026 •

edited

Loading

codecov Bot commented Aug 12, 2026 •

edited

Loading