From 2ea01e970fbc20661e9aa436742b7943469b7f7e Mon Sep 17 00:00:00 2001 From: "Matthew W. Thompson" Date: Fri, 13 Oct 2023 09:32:59 -0500 Subject: [PATCH 1/3] Do not allow `unique_molecules=None` when required --- openff/toolkit/topology/topology.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/openff/toolkit/topology/topology.py b/openff/toolkit/topology/topology.py index 19d243bb7..1f0bc0082 100644 --- a/openff/toolkit/topology/topology.py +++ b/openff/toolkit/topology/topology.py @@ -1314,7 +1314,7 @@ def _openmm_topology_to_networkx(openmm_topology): def from_openmm( cls, openmm_topology: "openmm.app.Topology", - unique_molecules: Optional[Iterable[FrozenMolecule]] = None, + unique_molecules: Iterable[FrozenMolecule], positions: Union[None, Quantity, "OMMQuantity"] = None, ) -> "Topology": """ @@ -1334,9 +1334,9 @@ def from_openmm( Parameters ---------- - openmm_topology + openmm_topology: openmm.app.Topology The OpenMM Topology object to convert - unique_molecules + unique_molecules: Iterable[FrozenMolecule] An iterable containing all the unique molecules in the topology. This is used to identify the molecules in the OpenMM topology and provide any missing chemical information. Each chemical species in @@ -1346,12 +1346,12 @@ def from_openmm( these reference molecules to the molecules appearing in the topology. If bond orders are specified in the topology, these will be used in matching as well. - positions + positions: optional, openff.unit.Quantity or openmm.unit.Quantity Positions for the atoms in the new topology. Returns ------- - topology + topology: openff.toolkit.Topology An OpenFF Topology object, constructed from the molecules in ``unique_molecules``, with the same atom order as the input topology. @@ -1540,7 +1540,7 @@ def _ensure_unique_atom_names(self, ensure_unique_atom_names: Union[str, bool]): def from_pdb( cls, file_path: Union[str, Path, TextIO], - unique_molecules: Optional[Iterable[Molecule]] = None, + unique_molecules: Iterable[Molecule] = tuple(), toolkit_registry=GLOBAL_TOOLKIT_REGISTRY, _custom_substructures: Optional[Dict[str, List[str]]] = None, _additional_substructures: Optional[Iterable[Molecule]] = None, @@ -1618,9 +1618,9 @@ def from_pdb( ---------- file_path : str, Path, or file object PDB information to be passed to OpenMM PDBFile object for loading - unique_molecules : Iterable of Molecule. Default = None + unique_molecules : Iterable of Molecule. Default = tuple() OpenFF Molecule objects corresponding to the molecules in the input - PDB. See above for details. + PDB. Can be empty. See above for details. toolkit_registry : ToolkitRegistry. Default = None The ToolkitRegistry to use as the cheminformatics backend. _custom_substructures: Dict[str, List[str]], Default = {} @@ -2120,7 +2120,7 @@ def set_positions(self, array: Quantity): def from_mdtraj( cls, mdtraj_topology: "mdtraj.Topology", - unique_molecules: Optional[Iterable[FrozenMolecule]] = None, + unique_molecules: Iterable[FrozenMolecule], positions: Union[None, "OMMQuantity", Quantity] = None, ): """ @@ -2136,9 +2136,9 @@ def from_mdtraj( Parameters ---------- - mdtraj_topology + mdtraj_topology: mdtraj.Topology The MDTraj Topology object to convert - unique_molecules + unique_molecules: Iterable[FrozenMolecule] An iterable containing all the unique molecules in the topology. This is used to identify the molecules in the MDTraj topology and provide any missing chemical information. Each chemical species in @@ -2148,12 +2148,12 @@ def from_mdtraj( these reference molecules to the molecules appearing in the topology. If bond orders are specified in the topology, these will be used in matching as well. - positions + positions: optional, openff.unit.Quantity or openmm.unit.Quantity Positions for the atoms in the new topology. Returns ------- - topology + topology: openff.toolkit.Topology An OpenFF Topology object, constructed from the molecules in ``unique_molecules``, with the same atom order as the input topology. From 53cdc6afcf595df2d9dd13688430f13c83f0ebeb Mon Sep 17 00:00:00 2001 From: "Matthew W. Thompson" Date: Thu, 22 Feb 2024 10:34:04 -0600 Subject: [PATCH 2/3] Remove `MissingUniqueMoleculesError` --- openff/toolkit/_tests/test_topology.py | 16 +++++++++------- openff/toolkit/topology/__init__.py | 2 -- openff/toolkit/topology/topology.py | 11 ----------- openff/toolkit/utils/exceptions.py | 6 ------ 4 files changed, 9 insertions(+), 26 deletions(-) diff --git a/openff/toolkit/_tests/test_topology.py b/openff/toolkit/_tests/test_topology.py index 4ba5904a4..03ffe0d15 100644 --- a/openff/toolkit/_tests/test_topology.py +++ b/openff/toolkit/_tests/test_topology.py @@ -64,7 +64,6 @@ InvalidBoxVectorsError, InvalidPeriodicityError, MissingConformersError, - MissingUniqueMoleculesError, MoleculeNotInTopologyError, NonUniqueSubstructureName, SubstructureAtomSmartsInvalid, @@ -507,11 +506,6 @@ def test_from_openmm(self): get_data_file_path("systems/packmol_boxes/cyclohexane_ethanol_0.4_0.6.pdb") ) - with pytest.raises( - MissingUniqueMoleculesError, match="requires a list of Molecule objects" - ): - Topology.from_openmm(pdbfile.topology) - molecules = [create_ethanol(), create_cyclohexane()] topology = Topology.from_openmm( @@ -529,6 +523,13 @@ def test_from_openmm(self): for omm_atom, off_atom in zip(pdbfile.topology.atoms(), topology.atoms): assert omm_atom.name == off_atom.name + def test_from_openmm_missing_unique_molecules(self): + with pytest.raises( + TypeError, + match="missing 1 required positional argument: 'unique_molecules'", + ): + Topology.from_openmm(Molecule.from_smiles("O").to_topology().to_openmm()) + def test_from_openmm_virtual_sites(self): from openff.toolkit import ForceField @@ -883,7 +884,8 @@ def test_from_mdtraj(self): trj = md.load(pdb_path) with pytest.raises( - MissingUniqueMoleculesError, match="requires a list of Molecule objects" + TypeError, + match="missing 1 required positional argument: 'unique_molecules'", ): Topology.from_mdtraj(trj.top) diff --git a/openff/toolkit/topology/__init__.py b/openff/toolkit/topology/__init__.py index 2c7a1aa24..9796f1aa1 100644 --- a/openff/toolkit/topology/__init__.py +++ b/openff/toolkit/topology/__init__.py @@ -12,7 +12,6 @@ ImproperDict, InvalidBoxVectorsError, InvalidPeriodicityError, - MissingUniqueMoleculesError, NotBondedError, SortedDict, TagSortedDict, @@ -31,7 +30,6 @@ "ImproperDict", "InvalidBoxVectorsError", "InvalidPeriodicityError", - "MissingUniqueMoleculesError", "NotBondedError", "SortedDict", "TagSortedDict", diff --git a/openff/toolkit/topology/topology.py b/openff/toolkit/topology/topology.py index 924974474..b137583c0 100644 --- a/openff/toolkit/topology/topology.py +++ b/openff/toolkit/topology/topology.py @@ -58,7 +58,6 @@ InvalidBoxVectorsError, InvalidPeriodicityError, MissingConformersError, - MissingUniqueMoleculesError, MoleculeNotInTopologyError, NotBondedError, VirtualSitesUnsupportedError, @@ -1363,8 +1362,6 @@ def from_openmm( Raises ------ - MissingUniqueMoleculesError - If ``unique_molecules`` is ``None`` DuplicateUniqueMoleculeError If the same connectivity graph is represented by two different molecules in ``unique_molecules`` @@ -1382,12 +1379,6 @@ def from_openmm( if omm_bond.order is None: omm_has_bond_orders = False - if unique_molecules is None: - raise MissingUniqueMoleculesError( - "Topology.from_openmm requires a list of Molecule objects " - "passed as unique_molecules, but None was passed." - ) - # Convert all unique mols to graphs topology = cls() graph_to_unq_mol: dict[Graph, FrozenMolecule] = {} @@ -2167,8 +2158,6 @@ def from_mdtraj( Raises ------ - MissingUniqueMoleculesError - If ``unique_molecules`` is ``None`` DuplicateUniqueMoleculeError If the same connectivity graph is represented by two different molecules in ``unique_molecules`` diff --git a/openff/toolkit/utils/exceptions.py b/openff/toolkit/utils/exceptions.py index e7709fbe3..16efbd13f 100644 --- a/openff/toolkit/utils/exceptions.py +++ b/openff/toolkit/utils/exceptions.py @@ -201,12 +201,6 @@ class InvalidPeriodicityError(OpenFFToolkitException): """ -class MissingUniqueMoleculesError(OpenFFToolkitException): - """ - Exception for when unique_molecules is required but not found - """ - - class SMIRKSMismatchError(OpenFFToolkitException): """ Exception for cases where smirks are inappropriate From c3610dd7102faf9f3723e9620090dc5325482370 Mon Sep 17 00:00:00 2001 From: "Matthew W. Thompson" Date: Thu, 22 Feb 2024 10:36:26 -0600 Subject: [PATCH 3/3] Update release history --- docs/releasehistory.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/releasehistory.md b/docs/releasehistory.md index 9261976d0..3ddbca5e4 100644 --- a/docs/releasehistory.md +++ b/docs/releasehistory.md @@ -10,6 +10,8 @@ Releases follow the `major.minor.micro` scheme recommended by [PEP440](https://w ### API-breaking changes +- [PR #1742](https://github.com/openforcefield/openff-toolkit/pull/1742): `Topology.from_openmm` and `Topology.from_mdtraj` now require `unique_molecules` at the level of the function signature. Previously `None` was allowed but would always throw `MissingUniqueMoleculesError`, which is now removed. + ### Behavior changes ### Bugfixes