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.30%. Comparing base (a4a5a6d) to head (76924a4).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2152      +/-   ##
==========================================
- Coverage   95.02%   90.30%   -4.73%     
==========================================
  Files         206      206              
  Lines       20514    20603      +89     
==========================================
- Hits        19493    18605     -888     
- Misses       1021     1998     +977

Flag	Coverage Δ
fast-tests	`90.30% <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

IAlibay · 2026-08-13T14:59:38Z

    RDKitToolkitWrapper,
 )
 from openff.units import unit
+from sympy.codegen.ast import continue_


Is this import necessary?

good catch not sure why pycharm pulled that in!

IAlibay · 2026-08-13T15:00:25Z

I liked your idea yesterday of having a "forcefield" entry to make it clear that this is the method that was being applied.

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:17:47Z

+            forcefields = [forcefields]
+
+        # this expects the full file name of the force field offxml file, e.g. "openff-2.0.0.offxml"
+        # which is different to how settings work which can leave off the .offxml extension


I think now we can use .offxml directly right? Should we change the defaults?

Also what happens if this encounters a non .offxml str? Should we try to add it?

Updated to accept both and added a test, and yes maybe the default should now have the extension as well.

Can you open a PR on gufe to update the default please?

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!

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-26T17:37:12Z

+            forcefields = [forcefields]
+
+        # this expects the full file name of the force field offxml file, e.g. "openff-2.0.0.offxml"
+        # which is different to how settings work which can leave off the .offxml extension


Can you open a PR on gufe to update the default please?

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

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

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] auto fixes from pre-commit.com hooks

76924a4

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

Uh oh!

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!

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!

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!

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!

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!

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!

Reviewers

jthorton commented Aug 12, 2026 •

edited

Loading

codecov Bot commented Aug 12, 2026 •

edited

Loading