diff --git a/openff/interchange/_tests/unit_tests/components/test_interchange.py b/openff/interchange/_tests/unit_tests/components/test_interchange.py index a0f281d0f..c771538ad 100644 --- a/openff/interchange/_tests/unit_tests/components/test_interchange.py +++ b/openff/interchange/_tests/unit_tests/components/test_interchange.py @@ -88,7 +88,7 @@ def test_missing_electrostatics_handler(self, tip3p, water): assert out["Electrostatics"].cutoff == 7.89 * unit.angstrom def test_box_setter(self): - tmp = Interchange() + tmp = Interchange(topology=Molecule.from_smiles("O").to_topology()) with pytest.raises(ValidationError): tmp.box = [2, 2, 3, 90, 90, 90] @@ -156,10 +156,10 @@ def test_from_sage(self, sage): def test_validate_simple_topology(self, sage): from openff.interchange.components.toolkit import _simple_topology_from_openmm - tmp = Interchange() - tmp.topology = _simple_topology_from_openmm( + topology = _simple_topology_from_openmm( Molecule.from_smiles("CCO").to_topology().to_openmm(), ) + Interchange(topology=topology) def test_from_sage_molecule_list(self, sage): out = Interchange.from_smirnoff( diff --git a/openff/interchange/_tests/unit_tests/interop/openmm/_import/test_import.py b/openff/interchange/_tests/unit_tests/interop/openmm/_import/test_import.py index 3d76f15d3..dc1129cf3 100644 --- a/openff/interchange/_tests/unit_tests/interop/openmm/_import/test_import.py +++ b/openff/interchange/_tests/unit_tests/interop/openmm/_import/test_import.py @@ -75,13 +75,19 @@ def test_different_ways_to_process_box_vectors( ): monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") + topology = Molecule.from_smiles("C").to_topology() + if as_argument: box = Interchange.from_openmm( system=simple_system, + topology=topology, box_vectors=simple_system.getDefaultPeriodicBoxVectors(), ).box else: - box = Interchange.from_openmm(system=simple_system).box + box = Interchange.from_openmm( + system=simple_system, + topology=topology, + ).box assert box.shape == (3, 3) diff --git a/openff/interchange/_tests/unit_tests/operations/test_combine.py b/openff/interchange/_tests/unit_tests/operations/test_combine.py index 9cf8d5fbf..f3dff074a 100644 --- a/openff/interchange/_tests/unit_tests/operations/test_combine.py +++ b/openff/interchange/_tests/unit_tests/operations/test_combine.py @@ -29,7 +29,7 @@ def test_basic_combination(self, monkeypatch, sage_unconstrained): interchange.positions = mol.conformers[0] # Copy and translate atoms by [1, 1, 1] - other = Interchange() + other = Interchange(topology=copy.deepcopy(interchange.topology)) other = copy.deepcopy(interchange) other.positions += 1.0 * unit.nanometer diff --git a/openff/interchange/components/interchange.py b/openff/interchange/components/interchange.py index 537af5c99..75d070f78 100644 --- a/openff/interchange/components/interchange.py +++ b/openff/interchange/components/interchange.py @@ -59,7 +59,7 @@ class Interchange(_BaseModel): """ collections: _AnnotatedCollections = Field(dict()) - topology: _AnnotatedTopology | None = Field(None) + topology: _AnnotatedTopology mdconfig: MDConfig | None = Field(None) box: _BoxQuantity | None = Field(None) # Needs shape/OpenMM validation positions: _PositionsQuantity | None = Field(None) # Ditto diff --git a/openff/interchange/foyer/_create.py b/openff/interchange/foyer/_create.py index 62ea5745c..4aa9a19d6 100644 --- a/openff/interchange/foyer/_create.py +++ b/openff/interchange/foyer/_create.py @@ -45,12 +45,11 @@ def _create_interchange( box: Quantity | None = None, positions: Quantity | None = None, ) -> Interchange: - interchange = Interchange() - _topology = Topology(topology) + interchange = Interchange(topology=Topology(topology)) - interchange.positions = _infer_positions(_topology, positions) + interchange.positions = _infer_positions(interchange.topology, positions) - interchange.box = _topology.box_vectors if box is None else box + interchange.box = interchange.topology.box_vectors if box is None else box # This block is from a mega merge, unclear if it's still needed for name, handler_class in get_handlers_callable().items(): @@ -58,7 +57,7 @@ def _create_interchange( vdw_handler = interchange["vdW"] vdw_handler.scale_14 = force_field.lj14scale - vdw_handler.store_matches(force_field, topology=_topology) + vdw_handler.store_matches(force_field, topology=interchange.topology) vdw_handler.store_potentials(force_field=force_field) atom_slots = vdw_handler.key_map @@ -72,16 +71,18 @@ def _create_interchange( for name, handler in interchange.collections.items(): if name not in ["vdW", "Electrostatics"]: - handler.store_matches(atom_slots, topology=_topology) + handler.store_matches(atom_slots, topology=interchange.topology) handler.store_potentials(force_field) # TODO: Populate .mdconfig, but only after a reasonable number of state mutations have been tested charges = electrostatics.charges - for molecule in _topology.molecules: + for molecule in interchange.topology.molecules: molecule_charges = [ - charges[TopologyKey(atom_indices=(_topology.atom_index(atom),))].m + charges[ + TopologyKey(atom_indices=(interchange.topology.atom_index(atom),)) + ].m for atom in molecule.atoms ] molecule.partial_charges = Quantity( @@ -92,6 +93,4 @@ def _create_interchange( interchange.collections["vdW"] = vdw_handler interchange.collections["Electrostatics"] = electrostatics - interchange.topology = _topology - return interchange diff --git a/openff/interchange/interop/gromacs/_interchange.py b/openff/interchange/interop/gromacs/_interchange.py index e951be061..b59f12233 100644 --- a/openff/interchange/interop/gromacs/_interchange.py +++ b/openff/interchange/interop/gromacs/_interchange.py @@ -292,7 +292,7 @@ def to_interchange( molecule_start_index += len(molecule_type.atoms) - interchange = Interchange() + interchange = Interchange(topology=_convert_topology(system)) interchange.collections.update( { @@ -307,7 +307,6 @@ def to_interchange( }, ) - interchange.topology = _convert_topology(system) interchange.positions = system.positions interchange.box = system.box diff --git a/openff/interchange/interop/openmm/_import/_import.py b/openff/interchange/interop/openmm/_import/_import.py index 17143621a..ddcfd79ff 100644 --- a/openff/interchange/interop/openmm/_import/_import.py +++ b/openff/interchange/interop/openmm/_import/_import.py @@ -37,15 +37,41 @@ def from_openmm( *, system: "openmm.System", + topology: Union["openmm.app.Topology", Topology], positions: Quantity | None = None, - topology: Union["openmm.app.Topology", Topology, None] = None, box_vectors: Quantity | None = None, ) -> "Interchange": """Create an Interchange object from OpenMM data.""" from openff.interchange import Interchange _check_compatible_inputs(system=system, topology=topology) - interchange = Interchange() + + if isinstance(topology, openmm.app.Topology): + from openff.units.openmm import from_openmm as from_openmm_ + + from openff.interchange.components.toolkit import _simple_topology_from_openmm + + openff_topology = _simple_topology_from_openmm(topology) + + if topology.getPeriodicBoxVectors() is not None: + openff_topology.box_vectors = from_openmm_(topology.getPeriodicBoxVectors()) + + # OpenMM topologies do not store positions + + elif isinstance(topology, Topology): + + openff_topology = topology + positions = openff_topology.get_positions() + + elif topology is None: + raise ValueError("A topology must be provided.") + + else: + raise ValueError( + f"Could not process `topology` argument of type {type(topology)=}.", + ) + + interchange = Interchange(topology=openff_topology) if system: constraints = _convert_constraints(system) @@ -74,22 +100,6 @@ def from_openmm( f"Unsupported OpenMM Force type ({type(force)}) found.", ) - if isinstance(topology, openmm.app.Topology): - from openff.interchange.components.toolkit import _simple_topology_from_openmm - - openff_topology = _simple_topology_from_openmm(topology) - - interchange.topology = openff_topology - - elif isinstance(topology, Topology): - - interchange.topology = topology - interchange.positions = topology.get_positions() - - elif topology is None: - - interchange.topology = topology - if positions is None: warnings.warn( @@ -104,29 +114,25 @@ def from_openmm( interchange.positions = positions if box_vectors is not None: - _box_vectors: Quantity = box_vectors - - elif topology is not None: - if isinstance(topology, openmm.app.Topology): - from openff.units.openmm import from_openmm as from_openmm_ + _box_vectors = box_vectors - _box_vectors = from_openmm_(topology.getPeriodicBoxVectors()) - elif isinstance(topology, Topology): - _box_vectors = topology.box_vectors + elif interchange.topology.box_vectors is not None: + _box_vectors = interchange.topology.box_vectors else: + # If there is no box argument passed and the topology is non-periodic + # and the system does not have default box vectors, it'll end up as None from openff.units.openmm import from_openmm as from_openmm_ _box_vectors = from_openmm_(system.getDefaultPeriodicBoxVectors()) - # TODO: There should probably be a more public box validator, checking for shape, etc. + # TODO: Does this run through the Interchange.box validator? interchange.box = _box_vectors - if interchange.topology is not None: - if interchange.topology.n_bonds > len(interchange.collections["Bonds"].key_map): - # There are probably missing (physics) bonds from rigid waters. The topological - # bonds are probably processed correctly. - _fill_in_rigid_water_bonds(interchange) + if interchange.topology.n_bonds > len(interchange.collections["Bonds"].key_map): + # There are probably missing (physics) bonds from rigid waters. The topological + # bonds are probably processed correctly. + _fill_in_rigid_water_bonds(interchange) return interchange diff --git a/openff/interchange/smirnoff/_create.py b/openff/interchange/smirnoff/_create.py index dab0787ed..d0329ef9b 100644 --- a/openff/interchange/smirnoff/_create.py +++ b/openff/interchange/smirnoff/_create.py @@ -107,41 +107,50 @@ def _create_interchange( _check_supported_handlers(force_field) - interchange = Interchange() + # interchange = Interchange(topology=topology) + # or maybe + interchange = Interchange(topology=validate_topology(topology)) - # TODO: Need to re-introduce logic lost when validator re-use was nuked - _topology = validate_topology(topology) + interchange.positions = _infer_positions(interchange.topology, positions) - interchange.positions = _infer_positions(_topology, positions) + interchange.box = interchange.topology.box_vectors if box is None else box - interchange.box = _topology.box_vectors if box is None else box - - _bonds(interchange, force_field, _topology, partial_bond_orders_from_molecules) + _bonds( + interchange, + force_field, + interchange.topology, + partial_bond_orders_from_molecules, + ) _constraints( interchange, force_field, - _topology, + interchange.topology, bonds=interchange.collections.get("Bonds", None), # type: ignore[arg-type] ) - _angles(interchange, force_field, _topology) - _propers(interchange, force_field, _topology, partial_bond_orders_from_molecules) - _impropers(interchange, force_field, _topology) + _angles(interchange, force_field, interchange.topology) + _propers( + interchange, + force_field, + interchange.topology, + partial_bond_orders_from_molecules, + ) + _impropers(interchange, force_field, interchange.topology) - _vdw(interchange, force_field, _topology) + _vdw(interchange, force_field, interchange.topology) _electrostatics( interchange, force_field, - _topology, + interchange.topology, charge_from_molecules, allow_nonintegral_charges, ) - _plugins(interchange, force_field, _topology) + _plugins(interchange, force_field, interchange.topology) - _virtual_sites(interchange, force_field, _topology) + _virtual_sites(interchange, force_field, interchange.topology) - _gbsa(interchange, force_field, _topology) + _gbsa(interchange, force_field, interchange.topology) - interchange.topology = _topology + interchange.topology = interchange.topology return interchange