Port #1216 (#1298)

* Fix #1216 (#1290) * change vsitehandler.find_matches to be more in theme with base find_matches return values * spurious print cleanup and releasenotes * fixes for mypy * Fix a type annotation * add test * black Co-authored-by: Matthew W. Thompson <mattwthompson@protonmail.com> * Update test given `VirtualSiteHandler.find_matches -> List` Co-authored-by: Jeff Wagner <jwagnerjpl@gmail.com>
openforcefield · May 9, 2022 · 41dd4ed · 41dd4ed
1 parent d29bd3f
commit 41dd4ed
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 16 deletions.
diff --git a/docs/releasehistory.md b/docs/releasehistory.md
@@ -121,7 +121,7 @@ print(value_roundtrip)
   [`Atom.symbol`](openff.toolkit.topology.molecule.Atom.symbol),
   [`Atom.mass`](openff.toolkit.topology.molecule.Atom.mass), and
   [`Atom.atomic_number`](openff.toolkit.topology.molecule.Atom.atomic_number).
-- [PR #1209](https://github.com/openforcefield/openforcefield/pull/1209): Fixes 
+- [PR #1209](https://github.com/openforcefield/openforcefield/pull/1209): Fixes
   [Issue #1073](https://github.com/openforcefield/openff-toolkit/issues/1073), where the
   `fractional_bondorder_method` kwarg to the 
   [`BondHandler`](openff.toolkit.typing.engines.smirnoff.parameters.BondHandler) initializer 
@@ -151,6 +151,14 @@ print(value_roundtrip)
 - [PR #1188](https://github.com/openforcefield/openff-toolkit/pull/1188): Add an `<Electrostatics>`
   section to the TIP3P force field file used in testing (`test_forcefields/tip3p.offxml`)
 
+### Minor bugfixes
+- [PR #1290](https://github.com/openforcefield/openforcefield/pull/1290): Fixes
+  [Issue #1216](https://github.com/openforcefield/openff-toolkit/issues/1216) by adding internal logic to handle
+  the possibility that multiple vsites share the same parent atom, and makes the return value of 
+  `VirtualSiteHandler.find_matches` be closer to the base class.
+
+
+
 ## 0.10.5 Bugfix release
 
 - [PR #1252](https://github.com/openforcefield/openforcefield/pull/1252): Refactors virtual 

diff --git a/openff/toolkit/tests/test_forcefield.py b/openff/toolkit/tests/test_forcefield.py
@@ -1907,6 +1907,51 @@ def test_hash_strip_ids(self):
 
         assert hash(ff_with_id) == hash(ff_without_id)
 
+    def test_issue_1216(self):
+        """Test that the conflict between vsitehandler and librarychargehandler
+        identified in issue #1216 remains resolved.
+
+        https://github.com/openforcefield/openff-toolkit/issues/1216
+        """
+        from openff.toolkit.typing.engines.smirnoff.parameters import VirtualSiteHandler
+
+        force_field = ForceField()
+        force_field.get_parameter_handler("Electrostatics")
+
+        vsite_handler: VirtualSiteHandler = force_field.get_parameter_handler(
+            "VirtualSites"
+        )
+        vsite_handler.add_parameter(
+            {
+                "smirks": "[#6:1][#9:2]",
+                "name": "EP",
+                "type": "BondCharge",
+                "distance": 1.0 * unit.angstrom,
+                "match": "all_permutations",
+                "charge_increment1": 0.2 * unit.elementary_charge,
+                "charge_increment2": 0.1 * unit.elementary_charge,
+                "sigma": 1.0 * unit.angstrom,
+                "epsilon": 0.0 * unit.kilocalorie_per_mole,
+            }
+        )
+
+        library_handler: LibraryChargeHandler = force_field.get_parameter_handler(
+            "LibraryCharges"
+        )
+        library_handler.add_parameter(
+            {
+                "smirks": "[F:2][C:1]([H:3])([H:4])([H:5])",
+                "charge": [
+                    0.3 * unit.elementary_charge,
+                    -0.15 * unit.elementary_charge,
+                    -0.05 * unit.elementary_charge,
+                    -0.05 * unit.elementary_charge,
+                    -0.05 * unit.elementary_charge,
+                ],
+            }
+        )
+        force_field.label_molecules(Molecule.from_smiles("CF").to_topology())
+
 
 class TestForceFieldChargeAssignment:
     @pytest.mark.parametrize(

diff --git a/openff/toolkit/tests/test_parameters.py b/openff/toolkit/tests/test_parameters.py
@@ -2340,11 +2340,11 @@ def test_find_matches(
 
         matched_smirks = defaultdict(set)
 
-        for match in matches:
-
-            matched_smirks[match.environment_match.topology_atom_indices].add(
-                (match.parameter_type.smirks, match.parameter_type.name)
-            )
+        for match_list in matches.values():
+            for match in match_list:
+                matched_smirks[match.environment_match.topology_atom_indices].add(
+                    (match.parameter_type.smirks, match.parameter_type.name)
+                )
 
         assert {**matched_smirks} == expected_matches
 
@@ -2370,11 +2370,13 @@ def test_find_matches_multiple_molecules(self):
 
         matched_smirks = defaultdict(set)
 
-        for match in matches:
+        for match_key in matches:
+            match_list: List = matches[match_key]
 
-            matched_smirks[match.environment_match.topology_atom_indices].add(
-                (match.parameter_type.smirks, match.parameter_type.name)
-            )
+            for match in match_list:
+                matched_smirks[match.environment_match.topology_atom_indices].add(
+                    (match.parameter_type.smirks, match.parameter_type.name)
+                )
 
         expected_matches = {
             (1, 0): {("[Cl:1]-[C:2]", "EP")},

diff --git a/openff/toolkit/typing/engines/smirnoff/forcefield.py b/openff/toolkit/typing/engines/smirnoff/forcefield.py
@@ -1441,8 +1441,8 @@ def label_molecules(self, topology):
         for molecule_idx, molecule in enumerate(topology.reference_molecules):
             top_mol = Topology.from_molecules([molecule])
             current_molecule_labels = dict()
-            param_is_list = False
             for tag, parameter_handler in self._parameter_handlers.items():
+                param_is_list = False
 
                 if type(parameter_handler) == VirtualSiteHandler:
                     param_is_list = True

diff --git a/openff/toolkit/typing/engines/smirnoff/parameters.py b/openff/toolkit/typing/engines/smirnoff/parameters.py
@@ -5241,22 +5241,22 @@ def _find_matches(
         entity: Topology,
         transformed_dict_cls=dict,
         unique=False,
-    ) -> List[ParameterHandler._Match]:
+    ) -> Dict[Tuple[int], List[ParameterHandler._Match]]:
 
         assigned_matches_by_parent = self._find_matches_by_parent(entity)
-        assigned_matches = []
-
+        return_dict = {}
         for parent_index, assigned_parameters in assigned_matches_by_parent.items():
-
+            assigned_matches = []
             for assigned_parameter, match_orientations in assigned_parameters:
 
                 for match in match_orientations:
 
                     assigned_matches.append(
                         ParameterHandler._Match(assigned_parameter, match)
                     )
+            return_dict[(parent_index,)] = assigned_matches
 
-        return assigned_matches
+        return return_dict
 
     def create_force(self, system, topology: Topology, **kwargs):
         raise NotImplementedError("Use `openff-interchange` instead.")