From cd183a85d95a4758b354af2766df320f4faea953 Mon Sep 17 00:00:00 2001 From: Josh Horton Date: Wed, 12 Aug 2026 11:31:33 +0100 Subject: [PATCH 01/14] try to get charges from the force field --- .../openmm_utils/charge_generation.py | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/src/openfe/protocols/openmm_utils/charge_generation.py b/src/openfe/protocols/openmm_utils/charge_generation.py index 332eda519..370adbb5d 100644 --- a/src/openfe/protocols/openmm_utils/charge_generation.py +++ b/src/openfe/protocols/openmm_utils/charge_generation.py @@ -11,7 +11,7 @@ import numpy as np from gufe import SmallMoleculeComponent -from openff.toolkit import Molecule as OFFMol +from openff.toolkit import Molecule as OFFMol, ForceField from openff.toolkit.utils.base_wrapper import ToolkitWrapper from openff.toolkit.utils.toolkit_registry import ToolkitRegistry from openff.toolkit.utils.toolkits import ( @@ -20,6 +20,7 @@ RDKitToolkitWrapper, ) from openff.units import unit +from sympy.codegen.ast import continue_ from threadpoolctl import threadpool_limits try: @@ -290,6 +291,7 @@ def assign_offmol_partial_charges( toolkit_backend: Literal["ambertools", "openeye", "rdkit"], generate_n_conformers: int | None, nagl_model: str | None, + force_field: list[str] | None = None, ) -> OFFMol: """ Assign partial charges to an OpenFF Molecule based on a selected method. @@ -299,7 +301,7 @@ def assign_offmol_partial_charges( offmol : openff.toolkit.Molecule The Molecule to assign partial charges to. overwrite : bool - Whether or not to overwrite any existing non-zero partial charges. + Whether to overwrite any existing non-zero partial charges. Note that zeroed charges will always be overwritten. method : Literal['am1bcc', 'am1bccelf10', 'nagl', 'espaloma'] Partial charge assignment method. @@ -319,6 +321,15 @@ def assign_offmol_partial_charges( nagl_model : str | None The NAGL model to use for charge assignment if method is ``nagl``. If ``None``, the latest am1bcc NAGL charge model is used. + force_field : list[str] | None, default None + An optional list of SMIRNOFF style force field offxml paths or strings which should be used to assign partial charges. + + Notes + ----- + Charges are applied based on the following source preferences: + - Charges already present on the ligand are retained if overwrite is ``False`` + - Charges are applied as defined by the method in the given SMIRNOFF style force field if present and the handler is not ``ToolkitAM1BCCHandler`` + - Charges are applied using the input method and settings Raises ------ @@ -339,6 +350,20 @@ def assign_offmol_partial_charges( if not overwrite: return offmol + if force_field is not None: + ff = ForceField(force_field) + if "ToolkitAM1BCC" in ff.registered_parameter_handlers: + ff.deregister_parameter_handler("ToolkitAM1BCC") + + try: + # let the force field resolve the partial charge assignment + charges = ff.get_partial_charges(offmol) + offmol.partial_charges = charges + return offmol + except RuntimeError: + # this has failed to assign charges fall back to the user specified method + pass + # Dictionary for each available charge method # The idea of this pattern is to allow for maximum flexibility by # allowing for swapping out method calls as necessary. From 46c5bbfebcad3c3764016ec747f9070555f51a73 Mon Sep 17 00:00:00 2001 From: Josh Horton Date: Wed, 19 Aug 2026 17:53:42 +0100 Subject: [PATCH 02/14] add forcefield option for charge assignment --- .../openmm_utils/charge_generation.py | 49 +++++++++------- .../tests/protocols/test_openmmutils.py | 56 +++++++++++++++++++ 2 files changed, 85 insertions(+), 20 deletions(-) diff --git a/src/openfe/protocols/openmm_utils/charge_generation.py b/src/openfe/protocols/openmm_utils/charge_generation.py index ceb557d0f..ff9581c8f 100644 --- a/src/openfe/protocols/openmm_utils/charge_generation.py +++ b/src/openfe/protocols/openmm_utils/charge_generation.py @@ -287,11 +287,11 @@ def _generate_offmol_conformers( def assign_offmol_partial_charges( offmol: OFFMol, overwrite: bool, - method: Literal["am1bcc", "am1bccelf10", "nagl", "espaloma"], + method: Literal["am1bcc", "am1bccelf10", "nagl", "espaloma", "forcefield"], toolkit_backend: Literal["ambertools", "openeye", "rdkit"], generate_n_conformers: int | None, nagl_model: str | None, - force_field: list[str] | None = None, + forcefields: list[str] | None = None, ) -> OFFMol: """ Assign partial charges to an OpenFF Molecule based on a selected method. @@ -303,9 +303,9 @@ def assign_offmol_partial_charges( overwrite : bool Whether to overwrite any existing non-zero partial charges. Note that zeroed charges will always be overwritten. - method : Literal['am1bcc', 'am1bccelf10', 'nagl', 'espaloma'] + method : Literal['am1bcc', 'am1bccelf10', 'nagl', 'espaloma', 'forcefield'] Partial charge assignment method. - Supported methods include; am1bcc, am1bccelf10, nagl, and espaloma. + Supported methods include; am1bcc, am1bccelf10, nagl, espaloma and forcefield. toolkit_backend : Literal['ambertools', 'openeye', 'rdkit'] OpenFF toolkit backend employed for charge generation. Supported options: @@ -321,15 +321,15 @@ def assign_offmol_partial_charges( nagl_model : str | None The NAGL model to use for charge assignment if method is ``nagl``. If ``None``, the latest am1bcc NAGL charge model is used. - force_field : list[str] | None, default None + forcefields : list[str] | None, default None An optional list of SMIRNOFF style force field offxml paths or strings which should be used to assign partial charges. Notes ----- Charges are applied based on the following source preferences: - Charges already present on the ligand are retained if overwrite is ``False`` - - Charges are applied as defined by the method in the given SMIRNOFF style force field if present and the handler is not ``ToolkitAM1BCCHandler`` - Charges are applied using the input method and settings + - the forcefield option will apply the default charges as intended by the force field. Raises ------ @@ -350,19 +350,24 @@ def assign_offmol_partial_charges( if not overwrite: return offmol - if force_field is not None: - ff = ForceField(force_field) - if "ToolkitAM1BCC" in ff.registered_parameter_handlers: - ff.deregister_parameter_handler("ToolkitAM1BCC") + if method.lower() == "forcefield": + if forcefields is None: + errmsg = ( + "The forcefield method requires a list of force fields to be provided " + "via `force_fields`." + ) + raise ValueError(errmsg) + + if isinstance(forcefields, str): + force_fields = [force_ields] - try: - # let the force field resolve the partial charge assignment - charges = ff.get_partial_charges(offmol) - offmol.partial_charges = charges - return offmol - except RuntimeError: - # this has failed to assign charges fall back to the user specified method - pass + # 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 + ff = ForceField(*forcefields) + # let the force field resolve the partial charge assignment + charges = ff.get_partial_charges(offmol) + offmol.partial_charges = charges + return offmol # Dictionary for each available charge method # The idea of this pattern is to allow for maximum flexibility by @@ -466,11 +471,12 @@ def assign_offmol_partial_charges( def bulk_assign_partial_charges( molecules: list[SmallMoleculeComponent], overwrite: bool, - method: Literal["am1bcc", "am1bccelf10", "nagl", "espaloma"], + method: Literal["am1bcc", "am1bccelf10", "nagl", "espaloma", "forcefield"], toolkit_backend: Literal["ambertools", "openeye", "rdkit"], generate_n_conformers: int | None, nagl_model: str | None, processors: int = 1, + forcefields: list[str] | None = None, ) -> list[SmallMoleculeComponent]: """ Assign partial charges to a list of SmallMoleculeComponents using multiprocessing. @@ -482,7 +488,7 @@ def bulk_assign_partial_charges( overwrite : bool Whether or not to overwrite any existing non-zero partial charges. Note that zeroed charges will always be overwritten. - method : Literal['am1bcc', 'am1bccelf10', 'nagl', 'espaloma'] + method : Literal['am1bcc', 'am1bccelf10', 'nagl', 'espaloma', 'forcefield] Partial charge assignment method. Supported methods include; am1bcc, am1bccelf10, nagl, and espaloma. toolkit_backend : Literal['ambertools', 'openeye', 'rdkit'] @@ -502,6 +508,8 @@ def bulk_assign_partial_charges( If ``None``, the latest am1bcc NAGL charge model is used. processors: int, default 1 The number of processors which should be used to generate the charges. + forcefields : list[str] | None, default None + An optional list of SMIRNOFF style force field offxml paths or strings which should be used to assign partial charges. Raises ------ @@ -524,6 +532,7 @@ def bulk_assign_partial_charges( "toolkit_backend": toolkit_backend, "generate_n_conformers": generate_n_conformers, "nagl_model": nagl_model, + "forcefields": forcefields, } if processors > 1: diff --git a/src/openfe/tests/protocols/test_openmmutils.py b/src/openfe/tests/protocols/test_openmmutils.py index f1672b6ba..eabe027a1 100644 --- a/src/openfe/tests/protocols/test_openmmutils.py +++ b/src/openfe/tests/protocols/test_openmmutils.py @@ -15,6 +15,7 @@ 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 @@ -1291,6 +1292,61 @@ def test_openeye_import_error(self, monkeypatch, uncharged_mol): nagl_model=None, ) + def test_forcefield_missing_ff(self, uncharged_mol): + # Make sure an error is raised if we forget to pass a force field to charge with + with pytest.raises(ValueError, match="The forcefield method requires a list of force fields to be provided via `force_fields`."): + charge_generation.assign_offmol_partial_charges( + uncharged_mol, + overwrite=False, + method="forcefield", + toolkit_backend="rdkit", + generate_n_conformers=None, + nagl_model=None, + ) + + def test_forcefield_charges_library(self, uncharged_mol): + # Make sure that the forcefield method can assign charges from a library + # Create a force field with a charge library for the molecule using a force field with an AM1BCC handler as well + ff = ForceField("openff-2.0.0.offxml") + lib_handler = ff.get_parameter_handler("LibraryCharges") + # add the new parameter + charged_mol = copy.deepcopy(uncharged_mol) + dummy_charges = np.zeros(charged_mol.n_atoms) * unit.e + # no other method should assign all zero charges + charged_mol.partial_charges = dummy_charges + lib_param = lib_handler._INFOTYPE.from_molecule(charged_mol) + lib_handler.add_parameter(parameter=lib_param) + del charged_mol + charge_generation.assign_offmol_partial_charges( + uncharged_mol, + overwrite=False, + method="forcefield", + toolkit_backend="rdkit", + generate_n_conformers=None, + nagl_model=None, + forcefields=ff.to_string(), + ) + + assert_allclose(uncharged_mol.partial_charges.m, dummy_charges.m) + + @pytest.mark.skipif(not HAS_NAGL, reason="NAGL is not available") + def test_forcefield_nagl_charges(self, uncharged_mol): + # Make sure that the forcefield method can assign charges from a NAGL model + charge_generation.assign_offmol_partial_charges( + uncharged_mol, + overwrite=False, + method="forcefield", + toolkit_backend="rdkit", + generate_n_conformers=None, + # set the model to none this should use the model define in the force field. + nagl_model=None, + # use a force field that has a NAGL handler and another redundant force field file + forcefields=["openff-2.3.0.offxml", "opc-1.0.0.offxml"], + ) + + assert uncharged_mol.partial_charges is not None + assert np.any(uncharged_mol.partial_charges) + @pytest.mark.slow @pytest.mark.skipif( From f372a16c40e16d6f0a93bf5a1fbd2e48e97bc43d Mon Sep 17 00:00:00 2001 From: Josh Horton Date: Wed, 19 Aug 2026 17:55:29 +0100 Subject: [PATCH 03/14] remove import --- src/openfe/protocols/openmm_utils/charge_generation.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/openfe/protocols/openmm_utils/charge_generation.py b/src/openfe/protocols/openmm_utils/charge_generation.py index ff9581c8f..82d44d58c 100644 --- a/src/openfe/protocols/openmm_utils/charge_generation.py +++ b/src/openfe/protocols/openmm_utils/charge_generation.py @@ -20,7 +20,6 @@ RDKitToolkitWrapper, ) from openff.units import unit -from sympy.codegen.ast import continue_ from threadpoolctl import threadpool_limits try: From 7c0575e82f9626c3b1400beb6e70bd930e6b5c87 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 19 Aug 2026 16:56:31 +0000 Subject: [PATCH 04/14] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/openfe/protocols/openmm_utils/charge_generation.py | 3 ++- src/openfe/tests/protocols/test_openmmutils.py | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/openfe/protocols/openmm_utils/charge_generation.py b/src/openfe/protocols/openmm_utils/charge_generation.py index 82d44d58c..f1c09faa5 100644 --- a/src/openfe/protocols/openmm_utils/charge_generation.py +++ b/src/openfe/protocols/openmm_utils/charge_generation.py @@ -11,7 +11,8 @@ import numpy as np from gufe import SmallMoleculeComponent -from openff.toolkit import Molecule as OFFMol, ForceField +from openff.toolkit import ForceField +from openff.toolkit import Molecule as OFFMol from openff.toolkit.utils.base_wrapper import ToolkitWrapper from openff.toolkit.utils.toolkit_registry import ToolkitRegistry from openff.toolkit.utils.toolkits import ( diff --git a/src/openfe/tests/protocols/test_openmmutils.py b/src/openfe/tests/protocols/test_openmmutils.py index eabe027a1..a11dd9cbb 100644 --- a/src/openfe/tests/protocols/test_openmmutils.py +++ b/src/openfe/tests/protocols/test_openmmutils.py @@ -14,8 +14,8 @@ from gufe.components.errors import ComponentValidationError 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 import Molecule as OFFMol from openff.toolkit.utils.toolkit_registry import ToolkitRegistry from openff.toolkit.utils.toolkits import RDKitToolkitWrapper from openff.units import unit @@ -1294,7 +1294,10 @@ def test_openeye_import_error(self, monkeypatch, uncharged_mol): def test_forcefield_missing_ff(self, uncharged_mol): # Make sure an error is raised if we forget to pass a force field to charge with - with pytest.raises(ValueError, match="The forcefield method requires a list of force fields to be provided via `force_fields`."): + with pytest.raises( + ValueError, + match="The forcefield method requires a list of force fields to be provided via `force_fields`.", + ): charge_generation.assign_offmol_partial_charges( uncharged_mol, overwrite=False, From 00692ec61006bef7ec811e38242428b5f47d425f Mon Sep 17 00:00:00 2001 From: Josh Horton Date: Wed, 19 Aug 2026 18:03:49 +0100 Subject: [PATCH 05/14] fix rename, update error message --- src/openfe/protocols/openmm_utils/charge_generation.py | 6 +++--- src/openfe/tests/protocols/test_openmmutils.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/openfe/protocols/openmm_utils/charge_generation.py b/src/openfe/protocols/openmm_utils/charge_generation.py index 82d44d58c..c36ca84ab 100644 --- a/src/openfe/protocols/openmm_utils/charge_generation.py +++ b/src/openfe/protocols/openmm_utils/charge_generation.py @@ -352,13 +352,13 @@ def assign_offmol_partial_charges( if method.lower() == "forcefield": if forcefields is None: errmsg = ( - "The forcefield method requires a list of force fields to be provided " - "via `force_fields`." + "The forcefield method requires a force field or list of force fields' to be provided " + "via `forcefields`." ) raise ValueError(errmsg) if isinstance(forcefields, str): - force_fields = [force_ields] + 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 diff --git a/src/openfe/tests/protocols/test_openmmutils.py b/src/openfe/tests/protocols/test_openmmutils.py index eabe027a1..1c16ada25 100644 --- a/src/openfe/tests/protocols/test_openmmutils.py +++ b/src/openfe/tests/protocols/test_openmmutils.py @@ -1294,7 +1294,7 @@ def test_openeye_import_error(self, monkeypatch, uncharged_mol): def test_forcefield_missing_ff(self, uncharged_mol): # Make sure an error is raised if we forget to pass a force field to charge with - with pytest.raises(ValueError, match="The forcefield method requires a list of force fields to be provided via `force_fields`."): + with pytest.raises(ValueError, match="The forcefield method requires a force field or list of force fields' to be provided via `forcefields`."): charge_generation.assign_offmol_partial_charges( uncharged_mol, overwrite=False, From 3eb3600406176019533ab213bdfa963b18a2dd5b Mon Sep 17 00:00:00 2001 From: Josh Horton Date: Thu, 20 Aug 2026 09:47:09 +0100 Subject: [PATCH 06/14] update cli option, add readme --- news/charge_from_ff.rst | 23 +++++++++++++++++++ .../parameters/plan_network_options.py | 2 ++ 2 files changed, 25 insertions(+) create mode 100644 news/charge_from_ff.rst diff --git a/news/charge_from_ff.rst b/news/charge_from_ff.rst new file mode 100644 index 000000000..ea8c4fbc1 --- /dev/null +++ b/news/charge_from_ff.rst @@ -0,0 +1,23 @@ +**Added:** + +* The ``assign_offmol_partial_charges`` and ``bulk_assign_partial_charges`` functions can assign charges from a list of OpenFF SMIRNOFF style force fields. Set method=``forcefield`` and provide a list of force field files via the new keyword argument ``forcefields``. This is also supported in the ``charge-molecules`` CLI command and is set by using a yaml settings file. + +**Changed:** + +* + +**Deprecated:** + +* + +**Removed:** + +* + +**Fixed:** + +* + +**Security:** + +* diff --git a/src/openfecli/parameters/plan_network_options.py b/src/openfecli/parameters/plan_network_options.py index b349d8972..b2b2a6871 100644 --- a/src/openfecli/parameters/plan_network_options.py +++ b/src/openfecli/parameters/plan_network_options.py @@ -223,6 +223,7 @@ def load_yaml_planner_options(path: Optional[str], context) -> PlanNetworkOption off_toolkit_backend: ambertools number_of_conformers: None nagl_model: None + forcefields: None """ _yaml_help = """ @@ -245,6 +246,7 @@ def load_yaml_planner_options(path: Optional[str], context) -> PlanNetworkOption - ``am1bccelf10`` (only possible if ``off_toolkit_backend`` is ``openeye``) - ``nagl`` (must have openff-nagl installed) - ``espaloma`` (must have espaloma_charge installed) + - ``forcefield`` (must supply the chosen force field files via the ``forcefields`` keyword argument. This is useful to get the correct AshGC model or LibraryCharges for a OpenFF force field.) ``settings:`` allows for passing in any keyword arguments of the method's corresponding Python API. From d7007026b93f0ad7d71518f54ffe6f7a0baac2af Mon Sep 17 00:00:00 2001 From: Josh Horton Date: Fri, 21 Aug 2026 11:52:51 +0100 Subject: [PATCH 07/14] finish CLI support and test, file extension handling --- .../openmm_utils/charge_generation.py | 26 ++++++-- .../protocols/openmm_utils/omm_settings.py | 11 +++- .../tests/protocols/test_openmmutils.py | 14 +++- .../commands/generate_partial_charges.py | 2 + .../tests/commands/test_charge_generation.py | 64 +++++++++++++++++++ 5 files changed, 108 insertions(+), 9 deletions(-) diff --git a/src/openfe/protocols/openmm_utils/charge_generation.py b/src/openfe/protocols/openmm_utils/charge_generation.py index 4796c177f..dea1fb0bd 100644 --- a/src/openfe/protocols/openmm_utils/charge_generation.py +++ b/src/openfe/protocols/openmm_utils/charge_generation.py @@ -327,9 +327,8 @@ def assign_offmol_partial_charges( Notes ----- Charges are applied based on the following source preferences: - - Charges already present on the ligand are retained if overwrite is ``False`` - - Charges are applied using the input method and settings - - the forcefield option will apply the default charges as intended by the force field. + - Charges already present on the ligand are retained if overwrite is ``False``. + - Charges are applied using the input method and settings. Raises ------ @@ -361,9 +360,24 @@ def assign_offmol_partial_charges( if isinstance(forcefields, str): 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 - ff = ForceField(*forcefields) + try: + # try to parse what the user has provided, due to supporting dropping the offxml extension, + # we need to try and catch the OSError and add the extension if needed + ff = ForceField(*forcefields) + except OSError: + # try adding the offxml extension if not present and it's a possible file path + forcefields_with_ext = [] + for _ff in forcefields: + # if the string of the force field is passed it should start with the xml header + if not _ff.endswith(".offxml") and not _ff.startswith("`_ Only ``ambertools`` and ``rdkit`` `off_toolkit_backend`` options are supported. A maximum of one conformer is allowed. + + ``forcefield``: + Assign partial charges using the OpenFF force field's defined charge model, this is useful to get the correct + NAGL(AshGC) model for a specific force field or to use LibraryCharges. """ off_toolkit_backend: Literal["ambertools", "openeye", "rdkit"] = "ambertools" @@ -314,6 +318,11 @@ class OpenFFPartialChargeSettings(BasePartialChargeSettings): If ``None`` (default) and ``partial_charge_method`` is set to ``nagl``, the latest available production am1bcc charge model will be used. """ + forcefields: list[str] | None = None + """ + An optional list of SMIRNOFF style force field offxml paths or strings which should be used to assign partial + charges if the ``partial_charge_method`` is set to ``forcefield``. + """ class OpenMMEngineSettings(SettingsBaseModel): diff --git a/src/openfe/tests/protocols/test_openmmutils.py b/src/openfe/tests/protocols/test_openmmutils.py index 953a643d8..4a7a22ba9 100644 --- a/src/openfe/tests/protocols/test_openmmutils.py +++ b/src/openfe/tests/protocols/test_openmmutils.py @@ -1335,6 +1335,7 @@ def test_forcefield_charges_library(self, uncharged_mol): @pytest.mark.skipif(not HAS_NAGL, reason="NAGL is not available") def test_forcefield_nagl_charges(self, uncharged_mol): # Make sure that the forcefield method can assign charges from a NAGL model + opc = ForceField("opc-1.0.0.offxml") charge_generation.assign_offmol_partial_charges( uncharged_mol, overwrite=False, @@ -1344,11 +1345,20 @@ def test_forcefield_nagl_charges(self, uncharged_mol): # set the model to none this should use the model define in the force field. nagl_model=None, # use a force field that has a NAGL handler and another redundant force field file - forcefields=["openff-2.3.0.offxml", "opc-1.0.0.offxml"], + # this is also testing that missing the file extension doesn't break the code + forcefields=["openff-2.3.0", opc.to_string()], ) assert uncharged_mol.partial_charges is not None - assert np.any(uncharged_mol.partial_charges) + + # get the reference charges to compare with + ff = ForceField("openff-2.3.0.offxml") + nagl_model = ff.get_parameter_handler("NAGLCharges").model_file + copy_mol = copy.deepcopy(uncharged_mol) + copy_mol.partial_charges = None + copy_mol.assign_partial_charges(partial_charge_method=nagl_model) + + assert_allclose(uncharged_mol.partial_charges.m, copy_mol.partial_charges.m, rtol=1e-4) @pytest.mark.slow diff --git a/src/openfecli/commands/generate_partial_charges.py b/src/openfecli/commands/generate_partial_charges.py index d1a83580f..3243263b9 100644 --- a/src/openfecli/commands/generate_partial_charges.py +++ b/src/openfecli/commands/generate_partial_charges.py @@ -14,6 +14,7 @@ - ``am1bccelf10`` (only possible if ``off_toolkit_backend`` is ``openeye``) - ``nagl`` (must have openff-nagl installed) - ``espaloma`` (must have espaloma_charge installed) + - ``forcefield`` (must supply the chosen force field files via the ``forcefields`` keyword argument.) ``settings`` allows for passing in any keyword arguments of the method's corresponding Python API. @@ -80,6 +81,7 @@ def charge_molecules(molecules, yaml_settings, output, n_cores, overwrite_charge generate_n_conformers=partial_charge.number_of_conformers, nagl_model=partial_charge.nagl_model, processors=n_cores, + forcefields=partial_charge.forcefields, ) write("\tDone") diff --git a/src/openfecli/tests/commands/test_charge_generation.py b/src/openfecli/tests/commands/test_charge_generation.py index d518a7ed7..26d07b8e3 100644 --- a/src/openfecli/tests/commands/test_charge_generation.py +++ b/src/openfecli/tests/commands/test_charge_generation.py @@ -16,6 +16,7 @@ HAS_OPENEYE, ) from openfecli.commands.generate_partial_charges import charge_molecules +import yaml @pytest.fixture @@ -188,3 +189,66 @@ def test_charge_settings( output_order.append(smc.name) assert input_order == output_order + + +def test_charge_molecules_missing_force_fields(methane, tmp_path): + # make sure an error is raised if we try to use forcefield charges without specifying a forcefield + runner = CliRunner() + mol_path = tmp_path / "methane.sdf" + methane.to_file(str(mol_path), "sdf") + out_path = str(tmp_path / "charged_methane.sdf") + + settings = { + "partial_charge": { + "method": "forcefield", + } + } + + settings_path = tmp_path / "settings.yaml" + yaml.safe_dump(settings, open(settings_path, "w")) + + with runner.isolated_filesystem(): + # # check an error is raised if we try to overwrite the input + with pytest.raises(ValueError, match="The forcefield method requires a force field or list of force fields' to be provided via `forcefields`."): + _ = runner.invoke( + charge_molecules, ["-M", mol_path, "-o", out_path, "-s", settings_path], catch_exceptions=False + ) + + +@pytest.mark.skipif( + not HAS_NAGL, + reason="needs NAGL", +) +@pytest.mark.skipif( + HAS_OPENEYE, reason="cannot use NAGL with rdkit backend when OpenEye is installed" +) +def test_charge_molecules_from_forcefield(methane, tmp_path): + # make sure we can use forcefield charges if we specify a forcefield + runner = CliRunner() + mol_path = tmp_path / "methane.sdf" + methane.to_file(str(mol_path), "sdf") + out_path = str(tmp_path / "charged_methane.sdf") + + settings = { + "partial_charge": { + "method": "forcefield", + "settings": { + "forcefields": ["openff_unconstrained-2.3.0"] + } + } + } + + settings_path = tmp_path / "settings.yaml" + yaml.safe_dump(settings, open(settings_path, "w")) + + with runner.isolated_filesystem(): + result = runner.invoke( + charge_molecules, ["-M", mol_path, "-o", out_path, "-s", settings_path], catch_exceptions=False + ) + assert result.exit_code == 0 + assert "Partial Charge Generation: forcefield" in result.output + + # make sure the charges have been saved + methane_out = SmallMoleculeComponent.from_sdf_file(filename=out_path) + off_methane_out = methane_out.to_openff() + assert off_methane_out.partial_charges is not None From 5083dd0e0a87f3a85665d0a56614bf3fecf5ef9a Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 21 Aug 2026 10:54:28 +0000 Subject: [PATCH 08/14] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- .../protocols/openmm_utils/omm_settings.py | 4 +++- .../tests/commands/test_charge_generation.py | 19 ++++++++++++------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/openfe/protocols/openmm_utils/omm_settings.py b/src/openfe/protocols/openmm_utils/omm_settings.py index 66f411b9f..562d292ab 100644 --- a/src/openfe/protocols/openmm_utils/omm_settings.py +++ b/src/openfe/protocols/openmm_utils/omm_settings.py @@ -238,7 +238,9 @@ class OpenFFPartialChargeSettings(BasePartialChargeSettings): Settings for controlling partial charge assignment using the OpenFF tooling """ - partial_charge_method: Literal["am1bcc", "am1bccelf10", "nagl", "espaloma", "forcefield"] = "am1bcc" + partial_charge_method: Literal["am1bcc", "am1bccelf10", "nagl", "espaloma", "forcefield"] = ( + "am1bcc" + ) """ Selection of method for partial charge generation. diff --git a/src/openfecli/tests/commands/test_charge_generation.py b/src/openfecli/tests/commands/test_charge_generation.py index 26d07b8e3..4d7d5ed58 100644 --- a/src/openfecli/tests/commands/test_charge_generation.py +++ b/src/openfecli/tests/commands/test_charge_generation.py @@ -3,6 +3,7 @@ import numpy as np import pytest +import yaml from click import ClickException from click.testing import CliRunner from gufe import SmallMoleculeComponent @@ -16,7 +17,6 @@ HAS_OPENEYE, ) from openfecli.commands.generate_partial_charges import charge_molecules -import yaml @pytest.fixture @@ -209,9 +209,14 @@ def test_charge_molecules_missing_force_fields(methane, tmp_path): with runner.isolated_filesystem(): # # check an error is raised if we try to overwrite the input - with pytest.raises(ValueError, match="The forcefield method requires a force field or list of force fields' to be provided via `forcefields`."): + with pytest.raises( + ValueError, + match="The forcefield method requires a force field or list of force fields' to be provided via `forcefields`.", + ): _ = runner.invoke( - charge_molecules, ["-M", mol_path, "-o", out_path, "-s", settings_path], catch_exceptions=False + charge_molecules, + ["-M", mol_path, "-o", out_path, "-s", settings_path], + catch_exceptions=False, ) @@ -232,9 +237,7 @@ def test_charge_molecules_from_forcefield(methane, tmp_path): settings = { "partial_charge": { "method": "forcefield", - "settings": { - "forcefields": ["openff_unconstrained-2.3.0"] - } + "settings": {"forcefields": ["openff_unconstrained-2.3.0"]}, } } @@ -243,7 +246,9 @@ def test_charge_molecules_from_forcefield(methane, tmp_path): with runner.isolated_filesystem(): result = runner.invoke( - charge_molecules, ["-M", mol_path, "-o", out_path, "-s", settings_path], catch_exceptions=False + charge_molecules, + ["-M", mol_path, "-o", out_path, "-s", settings_path], + catch_exceptions=False, ) assert result.exit_code == 0 assert "Partial Charge Generation: forcefield" in result.output From 38ff7c6739d514684290eb26ca5c91bd17777b33 Mon Sep 17 00:00:00 2001 From: Josh Horton Date: Fri, 21 Aug 2026 11:57:08 +0100 Subject: [PATCH 09/14] fix whitespace --- src/openfe/protocols/openmm_utils/omm_settings.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/openfe/protocols/openmm_utils/omm_settings.py b/src/openfe/protocols/openmm_utils/omm_settings.py index 562d292ab..37e883e02 100644 --- a/src/openfe/protocols/openmm_utils/omm_settings.py +++ b/src/openfe/protocols/openmm_utils/omm_settings.py @@ -272,9 +272,9 @@ class OpenFFPartialChargeSettings(BasePartialChargeSettings): `_ Only ``ambertools`` and ``rdkit`` `off_toolkit_backend`` options are supported. A maximum of one conformer is allowed. - + ``forcefield``: - Assign partial charges using the OpenFF force field's defined charge model, this is useful to get the correct + Assign partial charges using the OpenFF force field's defined charge model, this is useful to get the correct NAGL(AshGC) model for a specific force field or to use LibraryCharges. """ @@ -322,7 +322,7 @@ class OpenFFPartialChargeSettings(BasePartialChargeSettings): """ forcefields: list[str] | None = None """ - An optional list of SMIRNOFF style force field offxml paths or strings which should be used to assign partial + An optional list of SMIRNOFF style force field offxml paths or strings which should be used to assign partial charges if the ``partial_charge_method`` is set to ``forcefield``. """ From aec1d3597174a4f33494c7746fffb3c46d22db32 Mon Sep 17 00:00:00 2001 From: Josh Horton Date: Wed, 26 Aug 2026 10:05:38 +0100 Subject: [PATCH 10/14] use the toolkits in the input settings --- src/openfe/protocols/openmm_utils/charge_generation.py | 6 ++++-- src/openfe/protocols/openmm_utils/omm_settings.py | 4 +++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/openfe/protocols/openmm_utils/charge_generation.py b/src/openfe/protocols/openmm_utils/charge_generation.py index dea1fb0bd..54edcd8b6 100644 --- a/src/openfe/protocols/openmm_utils/charge_generation.py +++ b/src/openfe/protocols/openmm_utils/charge_generation.py @@ -378,8 +378,10 @@ def assign_offmol_partial_charges( # try again to load the force field with the added extension, if we fail let it raise the error ff = ForceField(*forcefields_with_ext) - # let the force field resolve the partial charge assignment - charges = ff.get_partial_charges(offmol) + # make the toolkit registry based on the selected backend + toolkits = ToolkitRegistry([i() for i in BACKEND_OPTIONS[toolkit_backend.lower()]]) + # let the force field resolve the partial charge assignment method + charges = ff.get_partial_charges(offmol, toolkit_registry=toolkits) offmol.partial_charges = charges return offmol diff --git a/src/openfe/protocols/openmm_utils/omm_settings.py b/src/openfe/protocols/openmm_utils/omm_settings.py index 37e883e02..cfa99c880 100644 --- a/src/openfe/protocols/openmm_utils/omm_settings.py +++ b/src/openfe/protocols/openmm_utils/omm_settings.py @@ -281,6 +281,8 @@ class OpenFFPartialChargeSettings(BasePartialChargeSettings): off_toolkit_backend: Literal["ambertools", "openeye", "rdkit"] = "ambertools" """ The OpenFF toolkit registry backend to use for partial charge generation. + + This is always respected regardless of the ``partial_charge_method``. OpenFF backend selection options @@ -322,7 +324,7 @@ class OpenFFPartialChargeSettings(BasePartialChargeSettings): """ forcefields: list[str] | None = None """ - An optional list of SMIRNOFF style force field offxml paths or strings which should be used to assign partial + An optional list of SMIRNOFF style force field offxml paths or raw force field contents strings which should be used to assign partial charges if the ``partial_charge_method`` is set to ``forcefield``. """ From 5616dc1693ddc85e0e32fdd8355aa3a9637bd320 Mon Sep 17 00:00:00 2001 From: Josh Horton Date: Wed, 26 Aug 2026 10:08:41 +0100 Subject: [PATCH 11/14] fix whitespace --- src/openfe/protocols/openmm_utils/omm_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openfe/protocols/openmm_utils/omm_settings.py b/src/openfe/protocols/openmm_utils/omm_settings.py index cfa99c880..0b8c575b8 100644 --- a/src/openfe/protocols/openmm_utils/omm_settings.py +++ b/src/openfe/protocols/openmm_utils/omm_settings.py @@ -281,7 +281,7 @@ class OpenFFPartialChargeSettings(BasePartialChargeSettings): off_toolkit_backend: Literal["ambertools", "openeye", "rdkit"] = "ambertools" """ The OpenFF toolkit registry backend to use for partial charge generation. - + This is always respected regardless of the ``partial_charge_method``. From 8c4421ffcea85cb0e82cdecc2e314ebe0c27d317 Mon Sep 17 00:00:00 2001 From: Josh Horton Date: Wed, 2 Sep 2026 12:59:35 +0100 Subject: [PATCH 12/14] PR feedback, update toolkit backends with nagl, add extra force field validation --- .../openmm_utils/charge_generation.py | 48 +++++++++++-------- .../tests/protocols/test_openmmutils.py | 18 ++++++- .../tests/commands/test_charge_generation.py | 2 +- 3 files changed, 47 insertions(+), 21 deletions(-) diff --git a/src/openfe/protocols/openmm_utils/charge_generation.py b/src/openfe/protocols/openmm_utils/charge_generation.py index 54edcd8b6..f78d26c71 100644 --- a/src/openfe/protocols/openmm_utils/charge_generation.py +++ b/src/openfe/protocols/openmm_utils/charge_generation.py @@ -70,6 +70,12 @@ "openeye": [OpenEyeToolkitWrapper], "rdkit": [RDKitToolkitWrapper], } +# If the user wants to use NAGL to assign charges via the force field option +# then we need to add it to the backend options if it is available +if HAS_NAGL: + BACKEND_OPTIONS["ambertools"].append(NAGLToolkitWrapper) + BACKEND_OPTIONS["openeye"].append(NAGLToolkitWrapper) + BACKEND_OPTIONS["rdkit"].append(NAGLToolkitWrapper) def assign_offmol_espaloma_charges(offmol: OFFMol, toolkit_registry: ToolkitRegistry) -> None: @@ -343,23 +349,27 @@ def assign_offmol_partial_charges( ------- The Molecule with partial charges assigned. """ + method_name = method.lower() + toolkit_name = toolkit_backend.lower() - # If you have non-zero charges and not overwriting, just return - if offmol.partial_charges is not None and np.any(offmol.partial_charges): - if not overwrite: - return offmol - - if method.lower() == "forcefield": + # validate the input combination first + if method_name == "forcefield": if forcefields is None: errmsg = ( "The forcefield method requires a force field or list of force fields' to be provided " "via `forcefields`." ) raise ValueError(errmsg) + elif forcefields is not None: + errmsg = f"The `forcefields` option is only valid with the `forcefield` charge method, but got {method_name}." + raise ValueError(errmsg) - if isinstance(forcefields, str): - forcefields = [forcefields] + # If you have non-zero charges and not overwriting, just return + if offmol.partial_charges is not None and np.any(offmol.partial_charges): + if not overwrite: + return offmol + if method_name == "forcefield": try: # try to parse what the user has provided, due to supporting dropping the offxml extension, # we need to try and catch the OSError and add the extension if needed @@ -379,7 +389,7 @@ def assign_offmol_partial_charges( ff = ForceField(*forcefields_with_ext) # make the toolkit registry based on the selected backend - toolkits = ToolkitRegistry([i() for i in BACKEND_OPTIONS[toolkit_backend.lower()]]) + toolkits = ToolkitRegistry([i() for i in BACKEND_OPTIONS[toolkit_name]]) # let the force field resolve the partial charge assignment method charges = ff.get_partial_charges(offmol, toolkit_registry=toolkits) offmol.partial_charges = charges @@ -430,13 +440,13 @@ def assign_offmol_partial_charges( # Grab the backends and also check our method try: - backends = CHARGE_METHODS[method.lower()]["backends"] + backends = CHARGE_METHODS[method_name]["backends"] except KeyError: errmsg = f"Unknown partial charge method {method}" raise ValueError(errmsg) # Check our method actually supports the toolkit backend selected - if toolkit_backend.lower() not in backends: # type: ignore + if toolkit_name not in backends: # type: ignore errmsg = ( f"Selected toolkit_backend ({toolkit_backend}) cannot " f"be used with the selected method ({method}). " @@ -445,26 +455,26 @@ def assign_offmol_partial_charges( raise ValueError(errmsg) # OpenEye is the only optional dependency in the toolkit backends - if toolkit_backend.lower() == "openeye" and not HAS_OPENEYE: + if toolkit_name == "openeye" and not HAS_OPENEYE: errmsg = "OpenEye is not available and cannot be selected as a backend" raise ImportError(errmsg) # Issue 1760 - if HAS_OPENEYE and method.lower() == "nagl": - if toolkit_backend.lower() != "openeye": + if HAS_OPENEYE and method_name == "nagl": + if toolkit_name != "openeye": errmsg = "OpenEye toolkit is installed but not used in the OpenFF toolkit registry backend. This is not possible with NAGL charges." raise ValueError(errmsg) - toolkits = ToolkitRegistry([i() for i in BACKEND_OPTIONS[toolkit_backend.lower()]]) + toolkits = ToolkitRegistry([i() for i in BACKEND_OPTIONS[toolkit_name]]) # We make a copy of the molecule since we're going to modify conformers offmol_copy = copy.deepcopy(offmol) # Generate conformers - note this method may differ based on the partial # charge method employed - CHARGE_METHODS[method.lower()]["confgen_func"]( + CHARGE_METHODS[method_name]["confgen_func"]( offmol=offmol_copy, - max_conf=CHARGE_METHODS[method.lower()]["max_conf"], + max_conf=CHARGE_METHODS[method_name]["max_conf"], toolkit_registry=toolkits, generate_n_conformers=generate_n_conformers, ) # type: ignore @@ -473,10 +483,10 @@ def assign_offmol_partial_charges( # with threadpool_limits(limits=1): # Call selected method to assign partial charges - CHARGE_METHODS[method.lower()]["charge_func"]( + CHARGE_METHODS[method_name]["charge_func"]( offmol=offmol_copy, toolkit_registry=toolkits, - **CHARGE_METHODS[method.lower()]["charge_extra_kwargs"], + **CHARGE_METHODS[method_name]["charge_extra_kwargs"], ) # type: ignore # Copy partial charges back diff --git a/src/openfe/tests/protocols/test_openmmutils.py b/src/openfe/tests/protocols/test_openmmutils.py index 4a7a22ba9..7da4e03c9 100644 --- a/src/openfe/tests/protocols/test_openmmutils.py +++ b/src/openfe/tests/protocols/test_openmmutils.py @@ -1307,6 +1307,22 @@ def test_forcefield_missing_ff(self, uncharged_mol): nagl_model=None, ) + def test_forcefields_wrong_method(self, uncharged_mol): + # Make sure an error is raised if we pass in some forcefields but the method isn't forcefield + with pytest.raises( + ValueError, + match="The `forcefields` option is only valid with the `forcefield` charge method, but got am1bcc.", + ): + charge_generation.assign_offmol_partial_charges( + uncharged_mol, + overwrite=False, + method="am1bcc", + toolkit_backend="rdkit", + generate_n_conformers=None, + nagl_model=None, + forcefields=["openff-2.0.0.offxml"], + ) + def test_forcefield_charges_library(self, uncharged_mol): # Make sure that the forcefield method can assign charges from a library # Create a force field with a charge library for the molecule using a force field with an AM1BCC handler as well @@ -1327,7 +1343,7 @@ def test_forcefield_charges_library(self, uncharged_mol): toolkit_backend="rdkit", generate_n_conformers=None, nagl_model=None, - forcefields=ff.to_string(), + forcefields=[ff.to_string()], ) assert_allclose(uncharged_mol.partial_charges.m, dummy_charges.m) diff --git a/src/openfecli/tests/commands/test_charge_generation.py b/src/openfecli/tests/commands/test_charge_generation.py index 4d7d5ed58..9fcb27322 100644 --- a/src/openfecli/tests/commands/test_charge_generation.py +++ b/src/openfecli/tests/commands/test_charge_generation.py @@ -237,7 +237,7 @@ def test_charge_molecules_from_forcefield(methane, tmp_path): settings = { "partial_charge": { "method": "forcefield", - "settings": {"forcefields": ["openff_unconstrained-2.3.0"]}, + "settings": {"forcefields": ["openff-2.3.0"]}, } } From 76924a44ded845e179569de012f0d7077beb6fa2 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:00:13 +0000 Subject: [PATCH 13/14] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/openfe/tests/protocols/test_openmmutils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openfe/tests/protocols/test_openmmutils.py b/src/openfe/tests/protocols/test_openmmutils.py index 7da4e03c9..b422f213f 100644 --- a/src/openfe/tests/protocols/test_openmmutils.py +++ b/src/openfe/tests/protocols/test_openmmutils.py @@ -1321,7 +1321,7 @@ def test_forcefields_wrong_method(self, uncharged_mol): generate_n_conformers=None, nagl_model=None, forcefields=["openff-2.0.0.offxml"], - ) + ) def test_forcefield_charges_library(self, uncharged_mol): # Make sure that the forcefield method can assign charges from a library From 56104788a917f2290dd0eb493cf8049a948ba3e4 Mon Sep 17 00:00:00 2001 From: Josh Horton Date: Wed, 2 Sep 2026 17:17:34 +0100 Subject: [PATCH 14/14] fix mypy --- src/openfe/protocols/openmm_utils/charge_generation.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/openfe/protocols/openmm_utils/charge_generation.py b/src/openfe/protocols/openmm_utils/charge_generation.py index f78d26c71..8abf0651b 100644 --- a/src/openfe/protocols/openmm_utils/charge_generation.py +++ b/src/openfe/protocols/openmm_utils/charge_generation.py @@ -6,6 +6,7 @@ import copy import sys +import typing import warnings from typing import Callable, Literal @@ -370,6 +371,8 @@ def assign_offmol_partial_charges( return offmol if method_name == "forcefield": + # mypy can't tell we have already validated that forcefields is not None + forcefields = typing.cast(list[str], forcefields) try: # try to parse what the user has provided, due to supporting dropping the offxml extension, # we need to try and catch the OSError and add the extension if needed