Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
23 changes: 23 additions & 0 deletions news/charge_from_ff.rst
Original file line number Diff line number Diff line change
@@ -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:**

* <news item>

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
46 changes: 40 additions & 6 deletions src/openfe/protocols/openmm_utils/charge_generation.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import numpy as np
from gufe import SmallMoleculeComponent
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
Expand Down Expand Up @@ -286,10 +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"],
Comment thread
IAlibay marked this conversation as resolved.
toolkit_backend: Literal["ambertools", "openeye", "rdkit"],
generate_n_conformers: int | None,
nagl_model: str | None,
forcefields: list[str] | None = None,
) -> OFFMol:
"""
Assign partial charges to an OpenFF Molecule based on a selected method.
Expand All @@ -299,11 +301,11 @@ 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']
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:
Expand All @@ -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.
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 using the input method and settings
- the forcefield option will apply the default charges as intended by the force field.
Comment thread
IAlibay marked this conversation as resolved.
Outdated

Raises
------
Expand All @@ -339,6 +350,25 @@ def assign_offmol_partial_charges(
if not overwrite:
return offmol

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

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.

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.

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.

Add the reverse check and test.

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)

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

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.

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?

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.

Updated to accept both and added a test, and yes maybe the default should now have the extension 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.

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

ff = ForceField(*forcefields)
# let the force field resolve the partial charge assignment
charges = ff.get_partial_charges(offmol)

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.

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.

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.

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?

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.

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.

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.

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

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.

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?

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.

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

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.

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

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.

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

offmol.partial_charges = charges
return offmol

# 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.
Expand Down Expand Up @@ -441,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.
Expand All @@ -457,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']
Expand All @@ -477,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
------
Expand All @@ -499,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:
Expand Down
59 changes: 59 additions & 0 deletions src/openfe/tests/protocols/test_openmmutils.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from gufe.components.errors import ComponentValidationError
from gufe.settings import OpenMMSystemGeneratorFFSettings, ThermoSettings
from numpy.testing import assert_allclose, assert_equal
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
Expand Down Expand Up @@ -1291,6 +1292,64 @@ 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 force field or list of force fields' to be provided via `forcefields`.",
):
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)
Comment thread
IAlibay marked this conversation as resolved.
Outdated


@pytest.mark.slow
@pytest.mark.skipif(
Expand Down
2 changes: 2 additions & 0 deletions src/openfecli/parameters/plan_network_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment thread
IAlibay marked this conversation as resolved.

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.

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

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.

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!

"""

_yaml_help = """
Expand All @@ -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.

Expand Down
Loading