Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing out what partial charges could look over the CLI #1068

Merged
merged 28 commits into from
Jan 30, 2025
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
b716571
Testing out what partial charges could look over the CLI
IAlibay Jan 7, 2025
a5075eb
update that one thing
IAlibay Jan 7, 2025
020ba71
fix signature
IAlibay Jan 7, 2025
45b8025
fix up a few things
IAlibay Jan 7, 2025
c5f0371
Merge branch 'main' into cli-partial-charges
jthorton Jan 16, 2025
283d9e9
add process pool, expose charge cli command
jthorton Jan 16, 2025
8383718
update flag name, fix tests to use nagl for CI speed
jthorton Jan 17, 2025
aa53762
Update openfecli/parameters/plan_network_options.py
jthorton Jan 20, 2025
c5e5900
Update openfecli/parameters/plan_network_options.py
jthorton Jan 20, 2025
e192f95
Update openfecli/parameters/plan_network_options.py
jthorton Jan 20, 2025
43efb18
Update openfecli/parameters/plan_network_options.py
jthorton Jan 20, 2025
8602a39
move ncores cli flag, fix tests, add tests for charge generation
jthorton Jan 20, 2025
c350c19
fix edge sort dependent tests, fix mypy
jthorton Jan 20, 2025
b3ef278
fix mypy, pre-charge ligands for reproducible tests
jthorton Jan 21, 2025
c2fc143
Update openfecli/commands/generate_partial_charges.py
jthorton Jan 23, 2025
d16bc3e
Update openfecli/commands/generate_partial_charges.py
jthorton Jan 23, 2025
99a891f
set threads to 1, undo tutorial file changes
jthorton Jan 23, 2025
73fa615
add news entry
jthorton Jan 23, 2025
37add05
update openff-toolkit pin
jthorton Jan 23, 2025
c46e7c1
add charges to cofactor sdf, fix docs, expose overwrite charges to pl…
jthorton Jan 24, 2025
6ab8e9f
fix test
jthorton Jan 24, 2025
4c9a1f0
xfail openeye nagl tests, update CLI ref docs
jthorton Jan 29, 2025
1da6c5a
Merge branch 'main' into cli-partial-charges
jthorton Jan 29, 2025
80b632c
Merge branch 'main' into cli-partial-charges
jthorton Jan 29, 2025
ea1589a
fix docs
jthorton Jan 29, 2025
fe5de58
Merge branch 'main' into cli-partial-charges
jthorton Jan 30, 2025
4bb2461
Merge branch 'main' into cli-partial-charges
atravitz Jan 30, 2025
1c2d40d
Merge branch 'main' into cli-partial-charges
atravitz Jan 30, 2025
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
120 changes: 110 additions & 10 deletions openfe/protocols/openmm_utils/charge_generation.py
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably another issue/PR - do we want to move this module somewhere else within the repo? It feels like it's more than just "openmm_utils".

Long term I think I want to move it to "openfe-utils" or similar, that way upstream protocols can use them without depending on OpenFE.

Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import sys
import warnings
import numpy as np
from gufe import SmallMoleculeComponent
from openff.units import unit
from openff.toolkit import Molecule as OFFMol
from openff.toolkit.utils.base_wrapper import ToolkitWrapper
Expand All @@ -17,6 +18,7 @@
RDKitToolkitWrapper
)
from openff.toolkit.utils.toolkit_registry import ToolkitRegistry
from openfe.utils.context_managers import temp_env

try:
import openeye
Expand Down Expand Up @@ -285,7 +287,7 @@ def assign_offmol_partial_charges(
toolkit_backend: Literal['ambertools', 'openeye', 'rdkit'],
generate_n_conformers: Optional[int],
nagl_model: Optional[str],
) -> None:
) -> OFFMol:
"""
Assign partial charges to an OpenFF Molecule based on a selected method.

Expand All @@ -297,7 +299,7 @@ def assign_offmol_partial_charges(
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']
Partial charge assignement method.
Partial charge assignment method.
Supported methods include; am1bcc, am1bccelf10, nagl, and espaloma.
toolkit_backend : Literal['ambertools', 'openeye', 'rdkit']
OpenFF toolkit backend employed for charge generation.
Expand All @@ -318,17 +320,21 @@ def assign_offmol_partial_charges(
Raises
------
ValueError
If the ``toolkit_backend`` is not suported by the selected ``method``.
If the ``toolkit_backend`` is not supported by the selected ``method``.
If ``generate_n_conformers`` is ``None``, but the input ``offmol``
has no associated conformers.
If the number of conformers passed or generated exceeds the number
of conformers selected by the partial charge ``method``.

Returns
-------
The Molecule with partial charges assigned.
"""

# 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
return offmol
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably going to silently break a bunch of things (particlularly protocols).

Would it be worth doing an inplace style flag where we optionally just overwrite the input molecule and otherwise return a molecule?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that as we still assign the charges to the input molecule and not a copy this should still be fine. I tried to make a small example of this here, which should hopefully mean we don't break anything!

from openff.toolkit import Molecule

def charge_mol(mol) -> Molecule:
    mol.assign_partial_charges("am1bcc")
    return mol

m = Molecule.from_smiles("CCO")
charge_mol(m)

print(m.partial_charges)

[-0.13610011111111114 0.12639988888888887 -0.5998001111111111 0.04236688888888887 0.04236688888888887 0.04236688888888887 0.04319988888888887 0.04319988888888887 0.3959998888888889] elementary_charge

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry - I think I hallucinated that you had removed the offmol_copy reassignment step below.

So just to confirm, this now does both an inplace asssignment and returns the offmol itself?

I guess, do we want to have the option to not do the inplace assignment? (i.e. could there be cases where this would be useful?)

Copy link
Collaborator

@jthorton jthorton Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it still does in place but I need the return for the multiprocessing to work, we can add an option for not in place but maybe that should be in another PR?


# Dictionary for each available charge method
# The idea of this pattern is to allow for maximum flexibility by
Expand Down Expand Up @@ -408,12 +414,106 @@ def assign_offmol_partial_charges(
generate_n_conformers=generate_n_conformers,
) # type: ignore

# Call selected method to assign partial charges
CHARGE_METHODS[method.lower()]['charge_func'](
offmol=offmol_copy,
toolkit_registry=toolkits,
**CHARGE_METHODS[method.lower()]['charge_extra_kwargs'],
) # type: ignore
# limit the number of threads used by SQM
# <https://github.com/openforcefield/openff-toolkit/issues/1831>
with temp_env({"OMP_NUM_THREADS": "2"}):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some more pythonic tools for thread limiting like this, let me have a dig in the morning.

i.e. we'll need them going forward, so maybe we should bite the bullet and go for it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what we use in MDAnalysis-land: https://github.com/joblib/threadpoolctl

It might be particularly helpful here because I'm not 100% sure all BLAS/LAPACK libraries respect OMP_NUM_THREADS (I'm thinking of you MKL library).

There might be better ones too!

# Call selected method to assign partial charges
CHARGE_METHODS[method.lower()]['charge_func'](
offmol=offmol_copy,
toolkit_registry=toolkits,
**CHARGE_METHODS[method.lower()]['charge_extra_kwargs'],
) # type: ignore

# Copy partial charges back
offmol.partial_charges = offmol_copy.partial_charges
return offmol


def bulk_assign_partial_charges(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Have you tested if it scales as intended?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we get a good speed up in local testing, charging 16 malt1 ligands (~42 atoms) with am1bcc (ambertools) takes:

  • 4:02 2 cores
  • 2:20 4 cores
  • 1: 48 6 cores

molecules: list[SmallMoleculeComponent],
overwrite: bool,
method: Literal['am1bcc', 'am1bccelf10', 'nagl', 'espaloma'],
toolkit_backend: Literal['ambertools', 'openeye', 'rdkit'],
generate_n_conformers: Optional[int],
nagl_model: Optional[str],
processors: int = 1,
) -> list[SmallMoleculeComponent]:
"""
Assign partial charges to a list of SmallMoleculeComponents using multiprocessing.

Parameters
----------
molecules : list[gufe.SmallMoleculeComponent]
The list of molecules who should have partial charges assigned.
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']
Partial charge assignment method.
Supported methods include; am1bcc, am1bccelf10, nagl, and espaloma.
toolkit_backend : Literal['ambertools', 'openeye', 'rdkit']
OpenFF toolkit backend employed for charge generation.
Supported options:
* ``ambertools``: selects both the AmberTools and RDKit Toolkit Wrapper
* ``openeye``: selects the OpenEye toolkit Wrapper
* ``rdkit``: selects the RDKit toolkit Wrapper
Note that the ``rdkit`` backend cannot be used for `am1bcc` or
``am1bccelf10`` partial charge methods.
generate_n_conformers : Optional[int]
Number of conformers to generate for partial charge generation.
If ``None`` (default), the input conformer will be used.
Values greater than 1 can only be used alongside ``am1bccelf10``.
nagl_model : Optional[str]
The NAGL model to use for charge assignment if method is ``nagl``.
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.

Raises
------
ValueError
If the ``toolkit_backend`` is not supported by the selected ``method``.
If ``generate_n_conformers`` is ``None``, but the input ``offmol``
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might actually be the case where we want to not inplace modify charges - what if things fails half way through a charge assignment loop, do we end up with have half our ligands charged and half our ligands uncharged?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If charging fails for one or more molecules the whole loop will break, so we might want to more gracefully handle failing though I am not sure what the end result would be one file with charged ligands and another with fails.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth testing - could be in a follow-up (for the sake of the CLI if this fails the whole thing fails, but the API is a bit different).

has no associated conformers.
If the number of conformers passed or generated exceeds the number
of conformers selected by the partial charge ``method``.

Returns
-------
A list of SmallMoleculeComponents with the charges assigned.
"""
import tqdm

charge_keywords = {
"overwrite": overwrite,
"method": method,
"toolkit_backend": toolkit_backend,
"generate_n_conformers": generate_n_conformers,
"nagl_model": nagl_model
}
charged_ligands = []

if processors > 1:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a reminder to add a test for this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 4c9a1f0

from concurrent.futures import ProcessPoolExecutor, as_completed
from multiprocessing import get_context

with ProcessPoolExecutor(max_workers=processors, mp_context=get_context("spawn")) as pool:

work_list = [
pool.submit(
assign_offmol_partial_charges,
m.to_openff(),
**charge_keywords,
)
for m in molecules
]

for work in tqdm.tqdm(as_completed(work_list), desc="Generating charges", ncols=80, total=len(molecules)):
charged_ligands.append(SmallMoleculeComponent.from_openff(work.result()))

else:
for m in tqdm.tqdm(molecules, desc="Generating charges", ncols=80, total=len(molecules)):
mol_with_charge = assign_offmol_partial_charges(m.to_openff(), **charge_keywords)
charged_ligands.append(SmallMoleculeComponent.from_openff(mol_with_charge))

return charged_ligands
25 changes: 25 additions & 0 deletions openfe/utils/context_managers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
from contextlib import contextmanager

@contextmanager
def temp_env(environment_variables: dict[str, str]):
"""
Temporarily update the global environment variables using the supplied values.

Inspired by bespokefit
<https://github.com/openforcefield/openff-bespokefit/blob/1bd79e9a9e4cea982153aed8e9cc6f8a37d65bd8/openff/bespokefit/utilities/_settings.py#L133>

Parameters
----------
environment_variables: dict[str, str]
The variables which should be changed while the manager is active.
"""
import os

old_env = dict(os.environ)
os.environ.update(environment_variables)

try:
yield
finally:
os.environ.clear()
os.environ.update(old_env)
92 changes: 92 additions & 0 deletions openfecli/commands/generate_partial_charges.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import click
from openfecli import OFECommandPlugin
from openfecli.parameters import MOL_DIR, YAML_OPTIONS, OUTPUT_FILE_AND_EXT, WORKERS


@click.command(
"charge-molecules",
short_help="Generate partial charges for a set of molecules."
)
@MOL_DIR.parameter(
required=True, help=MOL_DIR.kwargs["help"] + " Any number of sdf paths."
)
@YAML_OPTIONS.parameter(
multiple=False, required=False, default=None,
help=YAML_OPTIONS.kwargs["help"],
)
@OUTPUT_FILE_AND_EXT.parameter(
help="The name of the SDF file the charged ligands should be writen to."
jthorton marked this conversation as resolved.
Show resolved Hide resolved
)
@WORKERS.parameter(
help=WORKERS.kwargs["help"],
default=1,
)
@click.option(
"--overwrite-charges",
is_flag=True,
default=False,
help="If we should overwrite the charges already present in the molecules."
jthorton marked this conversation as resolved.
Show resolved Hide resolved
)
def charge_molecules(
molecules,
yaml_settings,
output,
workers,
overwrite_charges
):
"""
Generate partial charges for the set of input molecules and write them to file.
"""
from openfecli.utils import write
from openfe.protocols.openmm_utils.charge_generation import bulk_assign_partial_charges

write("SMALL MOLECULE PARTIAL CHARGE GENERATOR")
write("_________________________________________")
write("")

write("Parsing in Files: ")

# INPUT
write("\tGot input: ")

small_molecules = MOL_DIR.get(molecules)
write(
"\t\tSmall Molecules: "
+ " ".join([str(sm) for sm in small_molecules])
)

yaml_options = YAML_OPTIONS.get(yaml_settings)
partial_charge = yaml_options.partial_charge

write("Using Options:")
write("\tPartial Charge Generation: " + str(partial_charge.partial_charge_method))
write("")

charged_molecules = bulk_assign_partial_charges(
molecules=small_molecules,
overwrite=overwrite_charges,
method=partial_charge.partial_charge_method,
toolkit_backend=partial_charge.off_toolkit_backend,
generate_n_conformers=partial_charge.number_of_conformers,
nagl_model=partial_charge.nagl_model,
processors=workers
)

write("\tDone")
write("")

# OUTPUT
file, _ = OUTPUT_FILE_AND_EXT.get(output)
write("Output:")
write("\tSaving to: " + file.name)

# default is write bytes
file.mode = "w"
with file.open() as output:
for mol in charged_molecules:
output.write(mol.to_sdf())


PLUGIN = OFECommandPlugin(
command=charge_molecules, section="Miscellaneous", requires_ofe=(0, 3)
)
Loading
Loading