Merge pull request #844 from openforcefield/fix-charge-caching

Fix sensitivity to atom ordering in charge caching

docs/releasehistory.md

@@ -11,7 +11,11 @@ Dates are given in YYYY-MM-DD format.
 
 Please note that all releases prior to a version 1.0.0 are considered pre-releases and many API changes will come before a stable release.
 
-## Current development
+## 0.3.18 - 2023-11-16
+
+### Bugfixes
+
+* #844 Fixes a bug in which charge assignment caching incorrect charges between similar molecules with different atom orderings.
 
 ### Behavior changes
 

openff/interchange/_tests/unit_tests/smirnoff/test_nonbonded.py

@@ -1,6 +1,6 @@
 import numpy
 import pytest
-from openff.toolkit import Topology
+from openff.toolkit import Molecule, Topology
 from openff.toolkit.typing.engines.smirnoff import (
     ChargeIncrementModelHandler,
     ElectrostaticsHandler,
@@ -9,7 +9,7 @@
     vdWHandler,
 )
 from openff.toolkit.utils.exceptions import SMIRNOFFVersionError
-from openff.units import unit
+from openff.units import Quantity, unit
 from packaging.version import Version
 
 from openff.interchange import Interchange
@@ -202,6 +202,35 @@ def test_downconversion(self):
             pytest.skip("ParameterAttribute.__delete__ not implemented")
 
 
+class TestElectrostatics:
+    def test_caching_detects_atom_ordering(self, sage):
+        def get_charges_from_interchange(
+            molecule: Molecule,
+        ) -> dict[int, Quantity]:
+            return {
+                key.atom_indices[0]: val
+                for key, val in sage.create_interchange(molecule.to_topology())[
+                    "Electrostatics"
+                ].charges.items()
+            }
+
+        def compare_charges(
+            molecule: Molecule,
+            interchange_charges: dict[int, Quantity],
+        ):
+            for index, molecule_charge in enumerate(molecule.partial_charges):
+                assert interchange_charges[index] == molecule_charge
+
+        original = Molecule.from_mapped_smiles("[H:1]-[C:2]#[N:3]")
+        reordered = Molecule.from_mapped_smiles("[H:3]-[C:2]#[N:1]")
+
+        for molecule in [original, reordered]:
+            molecule.assign_partial_charges("am1bcc")
+
+        compare_charges(original, get_charges_from_interchange(original))
+        compare_charges(reordered, get_charges_from_interchange(reordered))
+
+
 class TestSMIRNOFFChargeIncrements(_BaseTest):
     @pytest.fixture()
     def hydrogen_cyanide_charge_increments(self):

openff/interchange/smirnoff/_nonbonded.py

@@ -456,7 +456,12 @@ def create(
 
     @classmethod
     @functools.lru_cache(None)
-    def _compute_partial_charges(cls, molecule: Molecule, method: str) -> "Quantity":
+    def _compute_partial_charges(
+        cls,
+        molecule: Molecule,
+        mapped_smiles: str,
+        method: str,
+    ) -> "Quantity":
         """Call out to the toolkit's toolkit wrappers to generate partial charges."""
         molecule = copy.deepcopy(molecule)
         molecule.assign_partial_charges(method)
@@ -646,6 +651,11 @@ def _find_charge_model_matches(
 
         partial_charges = cls._compute_partial_charges(
             unique_molecule,
+            unique_molecule.to_smiles(
+                isomeric=True,
+                explicit_hydrogens=True,
+                mapped=True,
+            ),
             method=partial_charge_method,
         )