Fix #1739 (#1756)

* fix #1739 * update vsite docs * black * update releasehistory --------- Co-authored-by: Matthew W. Thompson <mattwthompson@protonmail.com>
openforcefield · Nov 2, 2023 · 575501a · 575501a
1 parent 64621ae
commit 575501a
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 2 deletions.
diff --git a/docs/releasehistory.md b/docs/releasehistory.md
@@ -15,6 +15,7 @@ Releases follow the `major.minor.micro` scheme recommended by [PEP440](https://w
 ### Bugfixes
 
 - [PR #1740](https://github.com/openforcefield/openff-toolkit/pull/1740): Updates for Mypy 1.6.
+- [PR #1756](https://github.com/openforcefield/openff-toolkit/pull/1756): Fixes issue [#1739](https://github.com/openforcefield/openff-toolkit/issues/1739), where virtual sites would be double-created under some circumstances.
 
 ### New features
 

diff --git a/docs/users/virtualsites.md b/docs/users/virtualsites.md
@@ -118,8 +118,8 @@ site parameters. Let us consider 4-, 5-, and 6-point water models:
 
 ## Ordering of atoms and virtual sites
 
-The toolkit handles the orders the atoms and virtual sites in a topology in a
-specific manner for internal convenience.
+The OpenFF Toolkit and Interchange currently add all new virtual particles to the "end" of a Topology, 
+such that the particle indices of all newly-created virtual particles are higher than index of the last atom.
 
 In addition, due to the fact that a virtual site may contain multiple particles coupled 
 to single parameters, the toolkit makes a distinction between a virtual *site*, and a virtual 

diff --git a/openff/toolkit/_tests/test_parameters.py b/openff/toolkit/_tests/test_parameters.py
@@ -2425,6 +2425,29 @@ def test_invalid_num_charge_increments(self):
                 distance=2.0 * unit.angstrom,
             )
 
+    def test_deduplicate_symmetric_matches_in_noncapturing_atoms(self):
+        """
+        Make sure we don't double-assign vsites based on symmetries in non-tagged atoms in the SMIRKS.
+        See https://github.com/openforcefield/openff-toolkit/issues/1739
+        """
+        vsite_handler = VirtualSiteHandler(skip_version_check=True)
+        vsite_handler.add_parameter(
+            parameter_kwargs={
+                "smirks": "[H][#6:2]([H])=[#8:1]",
+                "name": "EP",
+                "type": "BondCharge",
+                "distance": 7.0 * openff.units.unit.angstrom,
+                "match": "all_permutations",
+                "charge_increment1": 0.2 * openff.units.unit.elementary_charge,
+                "charge_increment2": 0.1 * openff.units.unit.elementary_charge,
+                "sigma": 1.0 * openff.units.unit.angstrom,
+                "epsilon": 2.0 * openff.units.unit.kilocalorie_per_mole,
+            }
+        )
+        molecule = openff.toolkit.Molecule.from_mapped_smiles("[H:3][C:2]([H:4])=[O:1]")
+        matches = vsite_handler.find_matches(molecule.to_topology())
+        assert len(matches) == 1
+
 
 class TestLibraryChargeHandler:
     def test_create_library_charge_handler(self):

diff --git a/openff/toolkit/typing/engines/smirnoff/parameters.py b/openff/toolkit/typing/engines/smirnoff/parameters.py
@@ -3704,7 +3704,15 @@ def _find_matches_by_parent(self, entity: Topology) -> Dict[int, list]:
         matches_by_parent: Dict = defaultdict(lambda: defaultdict(list))
 
         for parameter in self._parameters:
+            # Filter for redundant matches caused by non-tagged atoms
+            # See https://github.com/openforcefield/openff-toolkit/issues/1739
+            seen_topology_atom_indices = set()
             for match in entity.chemical_environment_matches(parameter.smirks):
+                if match.topology_atom_indices in seen_topology_atom_indices:
+                    continue
+                else:
+                    seen_topology_atom_indices.add(match.topology_atom_indices)
+
                 parent_index = match.topology_atom_indices[parameter.parent_index]
 
                 matches_by_parent[parent_index][parameter.name].append(