Reproducing example:
from openff.toolkit.topology import Molecule
from openmm.app import ForceField
from openmmforcefields.generators import GAFFTemplateGenerator
molecule = Molecule.from_smiles("c1ccccc1") # benzene
generator = GAFFTemplateGenerator(molecules=molecule)
for repeat in range(2):
print(repeat)
forcefield = ForceField()
forcefield.registerTemplateGenerator(generator.generator)
system = forcefield.createSystem(molecule.to_topology().to_openmm())Output:
0
1
Traceback (most recent call last):
...
KeyError: 'ca'
I believe that the cause is an attempt to keep track of which atom types have been output so as not to output any duplicates:
|
# We need to make sure we don't duplicate atom types we have already told |
|
# OpenMM about. To do this we will keep track of atom types we have already |
|
# seen and ensure we only record new atom types in the xml. |
|
|
|
# iterate over a copy of the atom types |
|
for atom_type in params.atom_types.copy().keys(): |
|
if atom_type not in self._gaff_atom_types_observed: |
|
self._gaff_atom_types_observed.add(atom_type) |
|
_logger.debug(f"added {atom_type} to set of seen types") |
|
# if we have seen the atom type, delete it from the OG params, |
|
# not the copy! |
|
else: |
|
del params.atom_types[atom_type] |
|
_logger.debug(f"{atom_type} already recorded") |
But this is tracked on a per-generator basis, so when the generator is applied to the second force field, it doesn't tell it about any atom types, which of course fails. This should have been caught by the test suite, specifically
|
def test_multiple_registration(self): |
|
"""Test registering the template generator with multiple force fields""" |
which was designed to test exactly this, but unfortunately all the GAFF tests were disabled in commit
b685637 when GAFF was temporarily removed, and they were never reactivated in commit
051cbcb when GAFF was re-enabled (and the code leading to this problem was introduced).
@mikemhenry, @mattwthompson, or @peastman: For OpenMM >= 8.1.2, since duplicate registration of atom types is now allowed as long as they are identical thanks to openmm/openmm#4531, this logic should be unnecessary, right? But if we want to continue supporting older versions of OpenMM, or there's some other important reason for this logic, then something more complicated will have to be done.
Reproducing example:
Output:
I believe that the cause is an attempt to keep track of which atom types have been output so as not to output any duplicates:
openmmforcefields/openmmforcefields/generators/template_generators.py
Lines 737 to 750 in e6e6248
But this is tracked on a per-generator basis, so when the generator is applied to the second force field, it doesn't tell it about any atom types, which of course fails. This should have been caught by the test suite, specifically
openmmforcefields/openmmforcefields/tests/test_template_generators.py
Lines 823 to 824 in e6e6248
which was designed to test exactly this, but unfortunately all the GAFF tests were disabled in commit b685637 when GAFF was temporarily removed, and they were never reactivated in commit 051cbcb when GAFF was re-enabled (and the code leading to this problem was introduced).
@mikemhenry, @mattwthompson, or @peastman: For OpenMM >= 8.1.2, since duplicate registration of atom types is now allowed as long as they are identical thanks to openmm/openmm#4531, this logic should be unnecessary, right? But if we want to continue supporting older versions of OpenMM, or there's some other important reason for this logic, then something more complicated will have to be done.