Fix #837

[j-wags]

• Fix Use ELF10 for system creation when OpenEyeToolkitWrapper is available #837, making ToolkitAM1BCCHandler use ELF10 if OpenEye is available
• Add tests
• Lint codebase
• Update changelog

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (topology-biopolymer-refactor@fa03d80). Click here to learn what that means.
The diff coverage is n/a.

[mattwthompson]

LGTM. This is a behavior changed so I wanted to be thorough, but I couldn't find anything broken or surprising.

If I'm understanding the specification of this change correctly, it's simple: use amb1bccelf10 if OpenEye Toolkits are available, am1bcc otherwise. I created some systems locally with and without OpenEyeToolkitWrapper available in my development environment and this behavior is encoded as expected: the openmm.System has charges consistent molecule.assign_partial_charges(partial_charge_method="am1bccelf10") before uninstalling OpenEye and partial_charge_method="am1bcc" after.

I noticed that the ELF-10 variant of AM1-BCC is slower, at least when I call it directly on a molecule (presumably because it generates more conformers along the way?). I assume this was considered years ago when the SMIRNOFF spec was written with this behavior in mind. It just came as a small surprise to me.

The only issues I can imagine are out of scope here and arise from cases in which "OpenEye Toolkits are available" is not an identical condition to "OpenEyeToolkitWrapper is in the registry of toolkits". This assumption is already so heavily baked in everywhere that bigger problems would probably arise before we'd need to worry about ELF10 silently not being used - and adding any more testing around the license being available is, again, out of scope here.

[j-wags]

Thanks for the quick review!

The only issues I can imagine are out of scope here and arise from cases in which "OpenEye Toolkits are available" is not an identical condition to "OpenEyeToolkitWrapper is in the registry of toolkits". This assumption is already so heavily baked in everywhere that bigger problems would probably arise before we'd need to worry about ELF10 silently not being used - and adding any more testing around the license being available is, again, out of scope here.

Agreed. There are lots of complex failure cases possible with weird OE conditions. I think there was an idea a while back of having different decorators/checks for different OE products. If partially-available-OE-products end up causing a lot of trouble then we could consider committing a few days to fix that. But I'll wait for user reports since that would take a significant amount of time to implement.

[mattwthompson]

This has caused a minor regression in cases in which the ELF10 variant of AM1-BCC cannot assign partial charges but the vanilla variant can. Few and far between, I ran into this in a virtual site test using molecular oxygen (with OpenEye Toolkits installed and licensed):

>>> from openff.toolkit.topology import Molecule
>>> molecule = Molecule.from_smiles("O=O")
>>> molecule.assign_partial_charges(partial_charge_method="am1bcc")
>>> molecule.partial_charges
<Quantity([0. 0.], 'elementary_charge')>
>>> molecule.assign_partial_charges(partial_charge_method="am1bccelf10")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mwt/software/openforcefield/openff/toolkit/topology/molecule.py", line 3666, in assign_partial_charges
    toolkit_registry.call(
  File "/Users/mwt/software/openforcefield/openff/toolkit/utils/toolkit_registry.py", line 380, in call
    raise ValueError(msg)
ValueError: No registered toolkits can provide the capability "assign_partial_charges" for args "()" and kwargs "{'molecule': Molecule with name '' and SMILES 'O=O', 'partial_charge_method': 'am1bccelf10', 'use_conformers': None, 'strict_n_conformers': False, 'normalize_partial_charges': True, '_cls': <class 'openff.toolkit.topology.molecule.Molecule'>}"
Available toolkits are: [ToolkitWrapper around OpenEye Toolkit version 2021.2.0, ToolkitWrapper around The RDKit version 2021.09.5, ToolkitWrapper around AmberTools version 21.0, ToolkitWrapper around Built-in Toolkit version None]
 ToolkitWrapper around OpenEye Toolkit version 2021.2.0 <class 'openff.toolkit.utils.exceptions.ChargeCalculationError'> : Unable to assign charges: Warning: BCIChargeCorrector: BCI charge corrections for mol
Warning:                     missing bcc parameter for the O1-O2 bond (BCIType 310231)
Warning: Unable to assign all BCCs in

 ToolkitWrapper around The RDKit version 2021.09.5 <class 'openff.toolkit.utils.exceptions.ChargeMethodUnavailableError'> : partial_charge_method 'am1bccelf10' is not available from RDKitToolkitWrapper. Available charge methods are ['mmff94']
 ToolkitWrapper around AmberTools version 21.0 <class 'openff.toolkit.utils.exceptions.ChargeMethodUnavailableError'> : partial_charge_method 'am1bccelf10' is not available from AmberToolsToolkitWrapper. Available charge methods are ['am1bcc', 'am1-mulliken', 'gasteiger']
 ToolkitWrapper around Built-in Toolkit version None <class 'openff.toolkit.utils.exceptions.ChargeMethodUnavailableError'> : Partial charge method "am1bccelf10"" is not supported by the Built-in toolkit. Available charge methods are ['zeros', 'formal_charge']

It happens that we don't even have parameters to completely parametrize it:

>>> from openff.toolkit.typing.engines.smirnoff import ForceField
>>> sage = ForceField("openff-2.0.0.offxml")
>>> sage.create_openmm_system(molecule.to_topology())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mwt/software/openforcefield/openff/toolkit/utils/utils.py", line 85, in wrapper
    return function(*args, **kwargs)
  File "/Users/mwt/software/openforcefield/openff/toolkit/typing/engines/smirnoff/forcefield.py", line 1268, in create_openmm_system
    return self._old_create_openmm_system(topology, **kwargs)
  File "/Users/mwt/software/openforcefield/openff/toolkit/utils/utils.py", line 85, in wrapper
    return function(*args, **kwargs)
  File "/Users/mwt/software/openforcefield/openff/toolkit/typing/engines/smirnoff/forcefield.py", line 1358, in _old_create_openmm_system
    parameter_handler.create_force(system, topology, **kwargs)
  File "/Users/mwt/miniconda3/envs/openff-dev/lib/python3.9/site-packages/openff/utilities/utilities.py", line 75, in wrapper
    return function(*args, **kwargs)
  File "/Users/mwt/software/openforcefield/openff/toolkit/typing/engines/smirnoff/parameters.py", line 2894, in create_force
    self._check_all_valence_terms_assigned(
  File "/Users/mwt/software/openforcefield/openff/toolkit/typing/engines/smirnoff/parameters.py", line 2434, in _check_all_valence_terms_assigned
    raise exception
openff.toolkit.utils.exceptions.UnassignedBondParameterException: BondHandler was not able to find parameters for the following valence terms:

- Topology indices (0, 1): names and elements ( O), ( O),

I don't know how many molecules exhibit this behavior. But I was able to assign partial charges before, and presumably still would in an environment that only had open source cheminformatics toolkits installed.

[j-wags]

Thanks for reporting - I was a little concerned about this but I've been waiting to see if we have such a case for a druglike molecule. Right now I don't think we support parameterizing O2 (the second output in your post above is missing a bond parameter, unrelated to charge assignment), but if a druglike molecule comes along that works with OE AM1BCC "vanilla"but not AM1BCC ELF19 we can build in some fallback behavior.

There were some tricky cases where ELF10 would sometimes fail for carboxylates but those should have been fixed in #1171.