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

Allow Openeye for charge generation #489

Open
jthorton opened this issue Jul 17, 2023 · 9 comments
Open

Allow Openeye for charge generation #489

jthorton opened this issue Jul 17, 2023 · 9 comments

Comments

@jthorton
Copy link
Collaborator

PR #479 removed the use of oechem which causes charge generation to go through ambertools sqm which is much slower than openeye and also leads to inconsistent charges as my bespokefit dihedrals are fit against elf10 charges. Would it be possible not to enforce this but allow the passing of some global toolkit registry which can be set using the context manager in the toolkit?

@IAlibay
Copy link
Member

IAlibay commented Jul 17, 2023

Duplicate of #480?

@IAlibay
Copy link
Member

IAlibay commented Jul 17, 2023

I will point out that the context manager folks keep pointing to is, as far as I can tell, both an undocumented and untested component of the OpenFF toolkit. We've been burned already with private methods getting killed off silently upstream. Ideally OpenFF needs to make this a true supported method before we can use it here.

@jthorton
Copy link
Collaborator Author

Good point about the manager in the toolkit causing issues down the line. Another option might be to extend the version implemented in #479 (

def without_oechem_backend():
) to do something similar to the toolkit version?

@richardjgowers
Copy link
Contributor

For context, the reason we have (for now) disabled OEChem backend is that we were getting an error where some molecules were generating different SMILES from rdkit and oechem so charge generation would fail.

I'm a little wary of directly playing with the GLOBAL_TOOLKIT_REGISTRY as it looks a little in flux

We could probably build you a backdoor to do let you use oechem in a hacky way for now

@jthorton not sure I follow your thoughts on toolkit version?

@IAlibay
Copy link
Member

IAlibay commented Jul 17, 2023

I'd prefer it if we didn't have to extend our maintenance burden unnecessarily. If someone can ask OpenFF to make that method official & supported that'd be great.

In any case, in order to offer user supported selections I think we'll need to move things into the protocol unit. If so, I will need to look at things after I'm back.

@IAlibay
Copy link
Member

IAlibay commented Jul 17, 2023

Note, rather than being hacky I would ask that we instead officially support users passing charges directly to SmallMoleculeComponents. It's something we need anyways. Once that's done we can easily just allow system generator to pick up those charges.

@jthorton
Copy link
Collaborator Author

jthorton commented Jul 17, 2023

For context, the reason we have (for now) disabled OEChem backend is that we were getting an error where some molecules were generating different SMILES from rdkit and oechem so charge generation would fail.

I see is this due to some sort of cache?

We could probably build you a backdoor to do let you use oechem in a hacky way for now

Thanks, but I should be able to use library charges to pass in the charges computed locally. Provided I use openmm/openmmforcefields#288

I would ask that we instead officially support users passing charges directly to SmallMoleculeComponents. It's something we need anyways. Once that's done we can easily just allow system generator to pick up those charges.

Is the idea here to allow user-generated charges like RESP which can't currently be generated on the fly as well as the openeye/ambertools am1bcc charges? If so (openmm/openmmforcefields#288) would also allow this as users can add molecule specific library charges to the forcefield which might be a work around until you can pass the charges through the system generator method.

@jthorton not sure I follow your thoughts on toolkit version?

I was thinking you could copy the context manager from the toolkit until its officially supported there but I agree that playing with the global toolkit is never fun.

@IAlibay
Copy link
Member

IAlibay commented Jul 17, 2023

The idea is that currently SMCs will read in charges stored in OpenFF Molecules if created from them. It's rather useful if you're playing around with charges using the OpenFF TK. All we need to do is make this official and add a check on SMC creation that either removes the charges by default (to allow bad things from happening), or allow users to use the input charges if they really want.

Re: passing serialised information through, that can be another entry point, but ideally I'd prefer not have folks jump through hoops if they already have the charges in their Molecule.

@richardjgowers
Copy link
Contributor

@jthorton the error with OEChem is we're storing molecules initially as an rdkit Mol, then creating an OpenFF Mol, then on charge generation this was converting to an OEChem Mol. It looks like there was a sanity check in to_oechem that the SMILES didn't change, we found cases where RDKit and OEChem disagreed on the stereo label on some atoms so you couldn't go RDKit->OEChem via OpenffTK. Traceback below.

'Traceback (most recent call last):\n  File "/opt/conda/lib/python3.10/site-packages/gufe/protocols/protocolunit.py", line 319, in execute\n    outputs = self._execute(context, **inputs)\n  File "/opt/conda/lib/python3.10/site-packages/openfe/protocols/openmm_rfe/equil_rfe_methods.py", line 674, in _execute\n    outputs = self.run(scratch_basepath=ctx.scratch, shared_basepath=ctx.shared)\n  File "/opt/conda/lib/python3.10/site-packages/openfe/protocols/openmm_rfe/equil_rfe_methods.py", line 414, in run\n    system_generator.create_system(offmol.to_topology().to_openmm(),\n  File "/opt/conda/lib/python3.10/site-packages/openmmforcefields/generators/system_generators.py", line 316, in create_system\n    self.template_generator.add_molecules(molecules)\n  File "/opt/conda/lib/python3.10/site-packages/openmmforcefields/generators/template_generators.py", line 125, in add_molecules\n    self._molecules.update( { molecule.to_smiles() : molecule for molecule in molecules } )\n  File "/opt/conda/lib/python3.10/site-packages/openmmforcefields/generators/template_generators.py", line 125, in <dictcomp>\n    self._molecules.update( { molecule.to_smiles() : molecule for molecule in molecules } )\n  File "/opt/conda/lib/python3.10/site-packages/openff/toolkit/topology/molecule.py", line 1619, in to_smiles\n    smiles = to_smiles_method(self, isomeric, explicit_hydrogens, mapped)\n  File "/opt/conda/lib/python3.10/site-packages/openff/toolkit/utils/openeye_wrapper.py", line 1769, in to_smiles\n    oemol = self.to_openeye(molecule)\n  File "/opt/conda/lib/python3.10/site-packages/openff/toolkit/utils/openeye_wrapper.py", line 1531, in to_openeye\n    oemol, off_to_oe_idx = self._connection_table_to_openeye(\n  File "/opt/conda/lib/python3.10/site-packages/cachetools/__init__.py", line 737, in wrapper\n    v = func(*args, **kwargs)\n  File "/opt/conda/lib/python3.10/site-packages/openff/toolkit/utils/openeye_wrapper.py", line 1426, in _connection_table_to_openeye\n    raise InconsistentStereochemistryError(\nopenff.toolkit.utils.exceptions.InconsistentStereochemistryError: Programming error: OpenEye atom stereochemistry assumptions failed. The atom in the oemol has stereochemistry None and the atom in the offmol has stereoheometry S.\n'

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

No branches or pull requests

3 participants