diff --git a/docs/releasehistory.md b/docs/releasehistory.md index 248b5b1f1..27832b913 100644 --- 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 index e46c69ae8..292c1c53a 100644 --- 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 index 7d8455e6f..9209c8680 100644 --- 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 index fb2560e86..4211989cf 100644 --- 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(