Skip to content

RESP Charges#1552

Open
tkramer-motion wants to merge 33 commits into
masterfrom
resp
Open

RESP Charges#1552
tkramer-motion wants to merge 33 commits into
masterfrom
resp

Conversation

@tkramer-motion

Copy link
Copy Markdown
Collaborator

Uses pyscf to compute RESP charges as an alternative to OpenEye's AM1BCC

@proteneer proteneer left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Two major things to discuss offline

  1. openff toolkit/recharge dependency - I think this is mostly from _elf generation.
  2. Auto3D/AIMNET dependency (which adds torch, torchvision, torchaudio).

Comment thread timemachine/ff/handlers/nonbonded.py Outdated
from Auto3D.ASE.geometry import opt_geometry
from jax import jit, vmap
from numpy.typing import NDArray
from openff.recharge.aromaticity import AromaticityModel

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

is this overwritten by from timemachine.ff.handlers.bcc_aromaticity import AromaticityModel down below?

Comment thread timemachine/ff/handlers/nonbonded.py Outdated
from numpy.typing import NDArray
from openff.recharge.aromaticity import AromaticityModel
from openff.recharge.utilities.molecule import extract_conformers
from openff.toolkit import unit, RDKitToolkitWrapper

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think unit here is just openmm.unit

Comment thread requirements.txt Outdated
molecule: Molecule = Molecule.from_rdkit(rdmol)
# Apply ELF conformer selection to get diverse, representative conformers
# This helps ensure charges are computed from a representative ensemble
molecule.apply_elf_conformer_selection(

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

ugh - I think this is the biggest dependency on the openff

Comment thread requirements.txt Outdated
Comment thread tests/test_handlers.py Outdated
Comment thread tests/test_handlers.py Outdated
cache_key = nonbonded.RESP
am1h = nonbonded.RESPHandler(smirks, params, props)
mol = Chem.AddHs(Chem.MolFromSmiles("C1CNCOC1F"))
AllChem.EmbedMolecule(mol)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If RESP to be independent of the initial conformer, we should explicitly be calling mol.RemoveAllConformers() beforehand as opposed to EmbedMolecule()

Comment thread tests/test_handlers.py
# assert vjp_fn(charges_adjoints) == None


def test_resp_parameterization():

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should add a test for determinism (either in the presence or absence of a seed) since the underlying algorithm uses a stochastic conformer generator. i.e. we want to make sure that calling RESP multiple items returns identical charges on fresh non-cached molecules.

Comment thread timemachine/ff/handlers/nonbonded.py Outdated
Comment thread timemachine/ff/handlers/nonbonded.py Outdated
from openff.toolkit import unit, RDKitToolkitWrapper
from openff.toolkit.topology import Molecule
from openff.toolkit.utils import AntechamberNotFoundError
from openff.units import Quantity

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

again, I think this is just what's in OpenMM

mol.build()

# Perform Restricted Hartree-Fock calculation on GPU
mf = scf.RHF(mol).to_gpu()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is there a meaningful speed difference of cpu vs gpu? We've never tested downstream timemachine code robustly in multi-CUDA context settings, and generally assumes that tm is the only library within the process that creates/initializes the underlying cuda context.

Comment thread timemachine/ff/handlers/nonbonded.py
partial_charges = np.mean(am1_partial_charges, axis=0)

# Ensure total charge equals the formal charge of the molecule
# Small numerical errors can accumulate, so we redistribute any discrepancy

@proteneer proteneer Jun 2, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

How small are we talking about here? Is the source of error numerical or algorithmic?

Numerical: using float32 vs float64 etc.
Algorithm: use of restraints, constraints, poor convergence, etc.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

And would it be better to do this on a per-individual-conformer basis first and then average?

is avg(spread(charges)) == spread(avg(charges)) i.e. do the operators spread and avg commute?

Comment thread timemachine/ff/handlers/nonbonded.py Outdated
params.pruneRmsThresh = 1.0
params.clearConfs = True

AllChem.EmbedMultipleConfs(mol, 800, params)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Document choice of 800 here?

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.

It is just using the same number of conformers the existing oechem code uses. I don't know the origin of that choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants