From 800276d69b7c60f604369a9f6e40f2dbf84ad691 Mon Sep 17 00:00:00 2001 From: "Matthew W. Thompson" Date: Tue, 19 Mar 2024 10:43:25 -0500 Subject: [PATCH 1/5] INTEROP: Error out when non-bonded settings mismatch --- .../unit_tests/components/test_interchange.py | 187 ++++++++++-------- openff/interchange/components/interchange.py | 8 + openff/interchange/components/mdconfig.py | 18 +- openff/interchange/exceptions.py | 4 + 4 files changed, 131 insertions(+), 86 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/components/test_interchange.py b/openff/interchange/_tests/unit_tests/components/test_interchange.py index 29df9fa78..f8e224c38 100644 --- a/openff/interchange/_tests/unit_tests/components/test_interchange.py +++ b/openff/interchange/_tests/unit_tests/components/test_interchange.py @@ -2,12 +2,11 @@ import numpy import pytest -from openff.toolkit.topology import Molecule, Topology +from openff.toolkit import ForceField, Molecule, Topology, unit from openff.toolkit.typing.engines.smirnoff.parameters import ( ElectrostaticsHandler, ParameterHandler, ) -from openff.units import unit from openff.utilities.testing import skip_if_missing from openff.interchange import Interchange @@ -19,6 +18,7 @@ ) from openff.interchange.drivers import get_openmm_energies from openff.interchange.exceptions import ( + CutoffMismatchError, ExperimentalFeatureException, InvalidTopologyError, MissingParameterHandlerError, @@ -100,85 +100,6 @@ def test_box_setter(self): with pytest.raises(ValidationError): tmp.box = [2, 2, 3, 90, 90, 90] - @skip_if_missing("openmm") - def test_basic_combination(self, monkeypatch, sage_unconstrained): - """Test basic use of Interchange.__add__() based on the README example""" - monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") - - mol = Molecule.from_smiles("C") - mol.generate_conformers(n_conformers=1) - top = Topology.from_molecules([mol]) - - interchange = Interchange.from_smirnoff(sage_unconstrained, top) - - interchange.box = [4, 4, 4] * numpy.eye(3) - interchange.positions = mol.conformers[0] - - # Copy and translate atoms by [1, 1, 1] - other = Interchange() - other = deepcopy(interchange) - other.positions += 1.0 * unit.nanometer - - combined = interchange.combine(other) - - # Just see if it can be converted into OpenMM and run - get_openmm_energies(combined) - - def test_parameters_do_not_clash(self, monkeypatch, sage_unconstrained): - monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") - - thf = Molecule.from_smiles("C1CCOC1") - ace = Molecule.from_smiles("CC(=O)O") - - thf.generate_conformers(n_conformers=1) - ace.generate_conformers(n_conformers=1) - - def make_interchange(molecule: Molecule) -> Interchange: - molecule.generate_conformers(n_conformers=1) - interchange = Interchange.from_smirnoff( - force_field=sage_unconstrained, - topology=[molecule], - ) - interchange.positions = molecule.conformers[0] - - return interchange - - thf_interchange = make_interchange(thf) - ace_interchange = make_interchange(ace) - complex_interchange = thf_interchange.combine(ace_interchange) - - thf_vdw = thf_interchange["vdW"].get_system_parameters() - ace_vdw = ace_interchange["vdW"].get_system_parameters() - add_vdw = complex_interchange["vdW"].get_system_parameters() - - numpy.testing.assert_equal(numpy.vstack([thf_vdw, ace_vdw]), add_vdw) - - # TODO: Ensure the de-duplication is maintained after exports - - def test_positions_setting(self, monkeypatch, sage): - """Test that positions exist on the result if and only if - both input objects have positions.""" - - monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") - - ethane = Molecule.from_smiles("CC") - methane = Molecule.from_smiles("C") - - ethane_interchange = Interchange.from_smirnoff( - sage, - [ethane], - ) - methane_interchange = Interchange.from_smirnoff(sage, [methane]) - - ethane.generate_conformers(n_conformers=1) - methane.generate_conformers(n_conformers=1) - - assert (methane_interchange.combine(ethane_interchange)).positions is None - methane_interchange.positions = methane.conformers[0] - assert (methane_interchange.combine(ethane_interchange)).positions is None - ethane_interchange.positions = ethane.conformers[0] - assert (methane_interchange.combine(ethane_interchange)).positions is not None - def test_input_topology_not_modified(self, sage): molecule = Molecule.from_smiles("CCO") molecule.generate_conformers(n_conformers=1) @@ -525,3 +446,107 @@ def test_minimize(self, simple_interchange): minimied_energy = get_openmm_energies(simple_interchange).total_energy assert minimied_energy < original_energy + + +class TestCombine: + @skip_if_missing("openmm") + def test_basic_combination(self, monkeypatch, sage_unconstrained): + """Test basic use of Interchange.__add__() based on the README example""" + monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") + + mol = Molecule.from_smiles("C") + mol.generate_conformers(n_conformers=1) + top = Topology.from_molecules([mol]) + + interchange = Interchange.from_smirnoff(sage_unconstrained, top) + + interchange.box = [4, 4, 4] * numpy.eye(3) + interchange.positions = mol.conformers[0] + + # Copy and translate atoms by [1, 1, 1] + other = Interchange() + other = deepcopy(interchange) + other.positions += 1.0 * unit.nanometer + + combined = interchange.combine(other) + + # Just see if it can be converted into OpenMM and run + get_openmm_energies(combined) + + def test_parameters_do_not_clash(self, monkeypatch, sage_unconstrained): + monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") + + thf = Molecule.from_smiles("C1CCOC1") + ace = Molecule.from_smiles("CC(=O)O") + + thf.generate_conformers(n_conformers=1) + ace.generate_conformers(n_conformers=1) + + def make_interchange(molecule: Molecule) -> Interchange: + molecule.generate_conformers(n_conformers=1) + interchange = Interchange.from_smirnoff( + force_field=sage_unconstrained, + topology=[molecule], + ) + interchange.positions = molecule.conformers[0] + + return interchange + + thf_interchange = make_interchange(thf) + ace_interchange = make_interchange(ace) + complex_interchange = thf_interchange.combine(ace_interchange) + + thf_vdw = thf_interchange["vdW"].get_system_parameters() + ace_vdw = ace_interchange["vdW"].get_system_parameters() + add_vdw = complex_interchange["vdW"].get_system_parameters() + + numpy.testing.assert_equal(numpy.vstack([thf_vdw, ace_vdw]), add_vdw) + + # TODO: Ensure the de-duplication is maintained after exports + + def test_positions_setting(self, monkeypatch, sage): + """Test that positions exist on the result if and only if + both input objects have positions.""" + + monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") + + ethane = Molecule.from_smiles("CC") + methane = Molecule.from_smiles("C") + + ethane_interchange = Interchange.from_smirnoff( + sage, + [ethane], + ) + methane_interchange = Interchange.from_smirnoff(sage, [methane]) + + ethane.generate_conformers(n_conformers=1) + methane.generate_conformers(n_conformers=1) + + assert (methane_interchange.combine(ethane_interchange)).positions is None + methane_interchange.positions = methane.conformers[0] + assert (methane_interchange.combine(ethane_interchange)).positions is None + ethane_interchange.positions = ethane.conformers[0] + assert (methane_interchange.combine(ethane_interchange)).positions is not None + + @pytest.mark.parametrize("handler", ["Electrostatics", "vdW"]) + def test_error_mismatched_cutoffs( + self, + monkeypatch, + sage, + basic_top, + handler, + ): + + monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") + + sage_modified = ForceField("openff-2.1.0.offxml") + + sage_modified[handler].cutoff *= 1.5 + + with pytest.raises( + CutoffMismatchError, + match=f"{handler} cutoffs do not match", + ): + sage.create_interchange(basic_top).combine( + sage_modified.create_interchange(basic_top), + ) diff --git a/openff/interchange/components/interchange.py b/openff/interchange/components/interchange.py index 452f0d983..bd2b5ccd0 100644 --- a/openff/interchange/components/interchange.py +++ b/openff/interchange/components/interchange.py @@ -947,6 +947,9 @@ def __add__(self, other: "Interchange") -> "Interchange": def combine(self, other: "Interchange") -> "Interchange": """Combine two Interchange objects. This method is unstable and not yet unsafe.""" from openff.interchange.components.toolkit import _combine_topologies + from openff.interchange.operations._combine import ( + _check_nonbonded_compatibility, + ) warnings.warn( "Interchange object combination is experimental and likely to produce " @@ -961,6 +964,11 @@ def combine(self, other: "Interchange") -> "Interchange": self_copy.topology = _combine_topologies(self.topology, other.topology) atom_offset = self.topology.n_atoms + _check_nonbonded_compatibility( + self, + other, + ) + # TODO: Test that charge cache is invalidated in each of these cases if "Electrostatics" in self_copy.collections: self_copy["Electrostatics"]._charges = dict() self_copy["Electrostatics"]._charges_cached = False diff --git a/openff/interchange/components/mdconfig.py b/openff/interchange/components/mdconfig.py index 8634a7097..60cfd87c9 100644 --- a/openff/interchange/components/mdconfig.py +++ b/openff/interchange/components/mdconfig.py @@ -291,11 +291,19 @@ def _get_coeffs_of_constrained_bonds_and_angles( if len(interchange["Angles"].key_map) > 0: lmp.write("angle_style hybrid harmonic\n") - if len(interchange["ProperTorsions"].key_map) > 0: - lmp.write("dihedral_style hybrid fourier\n") - - if len(interchange["ImproperTorsions"].key_map) > 0: - lmp.write("improper_style cvff\n") + try: + if len(interchange["ProperTorsions"].key_map) > 0: + lmp.write("dihedral_style hybrid fourier\n") + except LookupError: + # no torsions here + pass + + try: + if len(interchange["ImproperTorsions"].key_map) > 0: + lmp.write("improper_style cvff\n") + except LookupError: + # no impropers here + pass # TODO: LAMMPS puts this information in the "run" file. Should it live in MDConfig or not? scale_factors = { diff --git a/openff/interchange/exceptions.py b/openff/interchange/exceptions.py index 473a1d89d..a66645581 100644 --- a/openff/interchange/exceptions.py +++ b/openff/interchange/exceptions.py @@ -102,6 +102,10 @@ class UnsupportedCombinationError(InterchangeException): """General exception for something going wrong in Interchange object combination.""" +class CutoffMismatchError(UnsupportedCombinationError): + """Non-bonded cutoffs do not match.""" + + class PluginCompatibilityError(InterchangeException): """A plugin is incompatible with the current version of Interchange in the way it is called.""" From 80b860a2fbdb14be6a522c6beeecf07dab254ded Mon Sep 17 00:00:00 2001 From: "Matthew W. Thompson" Date: Tue, 19 Mar 2024 11:58:39 -0500 Subject: [PATCH 2/5] REF: Move combination logic into a new file --- openff/interchange/components/interchange.py | 84 +------------ openff/interchange/operations/_combine.py | 117 +++++++++++++++++++ setup.cfg | 1 + 3 files changed, 120 insertions(+), 82 deletions(-) create mode 100644 openff/interchange/operations/_combine.py diff --git a/openff/interchange/components/interchange.py b/openff/interchange/components/interchange.py index bd2b5ccd0..eb96e6ecc 100644 --- a/openff/interchange/components/interchange.py +++ b/openff/interchange/components/interchange.py @@ -29,7 +29,6 @@ InvalidTopologyError, MissingParameterHandlerError, MissingPositionsError, - UnsupportedCombinationError, UnsupportedExportError, ) from openff.interchange.operations.minimize import ( @@ -946,88 +945,9 @@ def __add__(self, other: "Interchange") -> "Interchange": @experimental def combine(self, other: "Interchange") -> "Interchange": """Combine two Interchange objects. This method is unstable and not yet unsafe.""" - from openff.interchange.components.toolkit import _combine_topologies - from openff.interchange.operations._combine import ( - _check_nonbonded_compatibility, - ) - - warnings.warn( - "Interchange object combination is experimental and likely to produce " - "strange results. Any workflow using this method is not guaranteed to " - "be suitable for production. Use with extreme caution and thoroughly " - "validate results!", - stacklevel=2, - ) - - self_copy = copy.deepcopy(self) - - self_copy.topology = _combine_topologies(self.topology, other.topology) - atom_offset = self.topology.n_atoms - - _check_nonbonded_compatibility( - self, - other, - ) - # TODO: Test that charge cache is invalidated in each of these cases - if "Electrostatics" in self_copy.collections: - self_copy["Electrostatics"]._charges = dict() - self_copy["Electrostatics"]._charges_cached = False - - if "Electrostatics" in other.collections: - other["Electrostatics"]._charges = dict() - other["Electrostatics"]._charges_cached = False - - for handler_name, handler in other.collections.items(): - # TODO: Actually specify behavior in this case - try: - self_handler = self_copy.collections[handler_name] - except KeyError: - self_copy.collections[handler_name] = handler - warnings.warn( - f"'other' Interchange object has handler with name {handler_name} not " - f"found in 'self,' but it has now been added.", - stacklevel=2, - ) - continue - - for top_key, pot_key in handler.key_map.items(): - new_atom_indices = tuple( - idx + atom_offset for idx in top_key.atom_indices - ) - new_top_key = top_key.__class__(**top_key.dict()) - try: - new_top_key.atom_indices = new_atom_indices - except ValueError: - assert len(new_atom_indices) == 1 - new_top_key.this_atom_index = new_atom_indices[0] - - self_handler.key_map.update({new_top_key: pot_key}) - if handler_name == "Constraints": - self_handler.potentials.update( - {pot_key: handler.potentials[pot_key]}, - ) - else: - self_handler.potentials.update( - {pot_key: handler.potentials[pot_key]}, - ) - - self_copy.collections[handler_name] = self_handler - - if self_copy.positions is not None and other.positions is not None: - new_positions = np.vstack([self_copy.positions, other.positions]) - self_copy.positions = new_positions - else: - warnings.warn( - "Setting positions to None because one or both objects added together were missing positions.", - ) - self_copy.positions = None - - if not np.all(self_copy.box == other.box): - raise UnsupportedCombinationError( - "Combination with unequal box vectors is not curretnly supported", - ) + from openff.interchange.operations._combine import _combine - return self_copy + return _combine(self, other) def __repr__(self) -> str: periodic = self.box is not None diff --git a/openff/interchange/operations/_combine.py b/openff/interchange/operations/_combine.py new file mode 100644 index 000000000..62c088c60 --- /dev/null +++ b/openff/interchange/operations/_combine.py @@ -0,0 +1,117 @@ +"""The logic behind `Interchange.combine`.""" + +import copy +import warnings +from typing import TYPE_CHECKING + +import numpy + +from openff.interchange.components.toolkit import _combine_topologies +from openff.interchange.exceptions import ( + CutoffMismatchError, + UnsupportedCombinationError, +) + +if TYPE_CHECKING: + from openff.interchange.components.interchange import Interchange + + +def _check_nonbonded_compatibility( + interchange1: "Interchange", + interchange2: "Interchange", +): + if not ( + "vdW" in interchange1.handlers + and "vdW" in interchange2.handlers + and "Electrostatics" in interchange1.handlers + and "Electrostatics" in interchange2.handlers + ): + raise UnsupportedCombinationError( + "One or more inputs is missing a vdW and/or Electrostatics handler(s).", + ) + + for key in ["vdW", "Electrostatics"]: + if interchange1[key].cutoff != interchange2[key].cutoff: + raise CutoffMismatchError( + f"{key} cutoffs do not match. Found " + f"{interchange1[key].cutoff} and {interchange2[key].cutoff}.", + ) + + +def _combine( + input1: "Interchange", + input2: "Interchange", +) -> "Interchange": + + warnings.warn( + "Interchange object combination is experimental and likely to produce " + "strange results. Any workflow using this method is not guaranteed to " + "be suitable for production. Use with extreme caution and thoroughly " + "validate results!", + stacklevel=2, + ) + + result = copy.deepcopy(input1) + + result.topology = _combine_topologies(input1.topology, input2.topology) + atom_offset = input1.topology.n_atoms + + _check_nonbonded_compatibility(input1, input2) + + # TODO: Test that charge cache is invalidated in each of these cases + if "Electrostatics" in input1.collections: + input1["Electrostatics"]._charges = dict() + input1["Electrostatics"]._charges_cached = False + + if "Electrostatics" in input2.collections: + input2["Electrostatics"]._charges = dict() + input2["Electrostatics"]._charges_cached = False + + for handler_name, handler in input2.collections.items(): + # TODO: Actually specify behavior in this case + try: + self_handler = result.collections[handler_name] + except KeyError: + result.collections[handler_name] = handler + warnings.warn( + f"'other' Interchange object has handler with name {handler_name} not " + f"found in 'self,' but it has now been added.", + stacklevel=2, + ) + continue + + for top_key, pot_key in handler.key_map.items(): + new_atom_indices = tuple(idx + atom_offset for idx in top_key.atom_indices) + new_top_key = top_key.__class__(**top_key.dict()) + try: + new_top_key.atom_indices = new_atom_indices + except ValueError: + assert len(new_atom_indices) == 1 + new_top_key.this_atom_index = new_atom_indices[0] + + self_handler.key_map.update({new_top_key: pot_key}) + if handler_name == "Constraints": + self_handler.potentials.update( + {pot_key: handler.potentials[pot_key]}, + ) + else: + self_handler.potentials.update( + {pot_key: handler.potentials[pot_key]}, + ) + + result.collections[handler_name] = self_handler + + if result.positions is not None and input2.positions is not None: + result.positions = numpy.vstack([result.positions, input2.positions]) + else: + warnings.warn( + "Setting positions to None because one or both objects added together were missing positions.", + ) + result.positions = None + + if not numpy.all(result.box == result.box): + raise UnsupportedCombinationError( + "Combination with unequal box vectors is not curretnly supported", + ) + + return result diff --git a/setup.cfg b/setup.cfg index 6ff1dd5a1..1a007f4de 100644 --- a/setup.cfg +++ b/setup.cfg @@ -24,6 +24,7 @@ per-file-ignores = openff/interchange/components/*.py:F821 openff/interchange/components/foyer.py:F401 openff/interchange/components/_packmol.py:W503 + openff/interchange/operations/_combine.py:W503 openff/interchange/_tests/data/*:INP001 plugins/*:INP001 From 480718f9e21e0523597cf71189f38db8348f154f Mon Sep 17 00:00:00 2001 From: "Matthew W. Thompson" Date: Tue, 19 Mar 2024 12:25:52 -0500 Subject: [PATCH 3/5] MAINT: Update dependency constraints --- .github/workflows/ci.yaml | 2 +- .github/workflows/examples.yaml | 2 +- devtools/conda-envs/examples_env.yaml | 9 +++++---- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 5ac7faf6d..6dbcabc5e 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -103,7 +103,7 @@ jobs: if: always() run: | python -m pytest $COV openff/interchange/ \ - -r fExs -nauto --durations=10 \ + -r fExs -n logical --durations=10 \ -m "slow or not slow" \ --ignore=openff/interchange/_tests/energy_tests/test_energies.py diff --git a/.github/workflows/examples.yaml b/.github/workflows/examples.yaml index c5b2397cc..4bf6061ca 100644 --- a/.github/workflows/examples.yaml +++ b/.github/workflows/examples.yaml @@ -70,7 +70,7 @@ jobs: - name: Run example notebooks if: always() run: | - python -m pytest --nbval-lax --dist loadscope -nauto \ + python -m pytest --nbval-lax --dist loadscope -n logical \ examples/ \ --ignore=examples/deprecated/ \ --ignore=examples/lammps/ \ diff --git a/devtools/conda-envs/examples_env.yaml b/devtools/conda-envs/examples_env.yaml index 401d017bf..26ad1a2a6 100644 --- a/devtools/conda-envs/examples_env.yaml +++ b/devtools/conda-envs/examples_env.yaml @@ -5,9 +5,9 @@ channels: dependencies: # Core - python - - numpy =1 - - pydantic =1 - - openmm >=7.6 + - numpy + - pydantic =2 + - openmm # OpenFF stack - openff-toolkit =0.15.2 - openff-models @@ -16,8 +16,9 @@ dependencies: - dgl =1 # Optional features - unyt - - mbuild + - mbuild =0.17 - foyer >=0.12.1 + - gmso =0.12 # Examples - jax - mdtraj From f20c8fdd478ae86b89b83e44283103e5d45f41e4 Mon Sep 17 00:00:00 2001 From: "Matthew W. Thompson" Date: Tue, 19 Mar 2024 12:37:29 -0500 Subject: [PATCH 4/5] INTEROP: Error out when switching function settings mismatch --- .../unit_tests/components/test_interchange.py | 109 +------------- .../unit_tests/operations/test_combine.py | 137 ++++++++++++++++++ openff/interchange/exceptions.py | 4 + openff/interchange/operations/_combine.py | 7 + 4 files changed, 149 insertions(+), 108 deletions(-) create mode 100644 openff/interchange/_tests/unit_tests/operations/test_combine.py diff --git a/openff/interchange/_tests/unit_tests/components/test_interchange.py b/openff/interchange/_tests/unit_tests/components/test_interchange.py index f8e224c38..03f8213f9 100644 --- a/openff/interchange/_tests/unit_tests/components/test_interchange.py +++ b/openff/interchange/_tests/unit_tests/components/test_interchange.py @@ -1,8 +1,6 @@ -from copy import deepcopy - import numpy import pytest -from openff.toolkit import ForceField, Molecule, Topology, unit +from openff.toolkit import Molecule, Topology, unit from openff.toolkit.typing.engines.smirnoff.parameters import ( ElectrostaticsHandler, ParameterHandler, @@ -18,7 +16,6 @@ ) from openff.interchange.drivers import get_openmm_energies from openff.interchange.exceptions import ( - CutoffMismatchError, ExperimentalFeatureException, InvalidTopologyError, MissingParameterHandlerError, @@ -446,107 +443,3 @@ def test_minimize(self, simple_interchange): minimied_energy = get_openmm_energies(simple_interchange).total_energy assert minimied_energy < original_energy - - -class TestCombine: - @skip_if_missing("openmm") - def test_basic_combination(self, monkeypatch, sage_unconstrained): - """Test basic use of Interchange.__add__() based on the README example""" - monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") - - mol = Molecule.from_smiles("C") - mol.generate_conformers(n_conformers=1) - top = Topology.from_molecules([mol]) - - interchange = Interchange.from_smirnoff(sage_unconstrained, top) - - interchange.box = [4, 4, 4] * numpy.eye(3) - interchange.positions = mol.conformers[0] - - # Copy and translate atoms by [1, 1, 1] - other = Interchange() - other = deepcopy(interchange) - other.positions += 1.0 * unit.nanometer - - combined = interchange.combine(other) - - # Just see if it can be converted into OpenMM and run - get_openmm_energies(combined) - - def test_parameters_do_not_clash(self, monkeypatch, sage_unconstrained): - monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") - - thf = Molecule.from_smiles("C1CCOC1") - ace = Molecule.from_smiles("CC(=O)O") - - thf.generate_conformers(n_conformers=1) - ace.generate_conformers(n_conformers=1) - - def make_interchange(molecule: Molecule) -> Interchange: - molecule.generate_conformers(n_conformers=1) - interchange = Interchange.from_smirnoff( - force_field=sage_unconstrained, - topology=[molecule], - ) - interchange.positions = molecule.conformers[0] - - return interchange - - thf_interchange = make_interchange(thf) - ace_interchange = make_interchange(ace) - complex_interchange = thf_interchange.combine(ace_interchange) - - thf_vdw = thf_interchange["vdW"].get_system_parameters() - ace_vdw = ace_interchange["vdW"].get_system_parameters() - add_vdw = complex_interchange["vdW"].get_system_parameters() - - numpy.testing.assert_equal(numpy.vstack([thf_vdw, ace_vdw]), add_vdw) - - # TODO: Ensure the de-duplication is maintained after exports - - def test_positions_setting(self, monkeypatch, sage): - """Test that positions exist on the result if and only if - both input objects have positions.""" - - monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") - - ethane = Molecule.from_smiles("CC") - methane = Molecule.from_smiles("C") - - ethane_interchange = Interchange.from_smirnoff( - sage, - [ethane], - ) - methane_interchange = Interchange.from_smirnoff(sage, [methane]) - - ethane.generate_conformers(n_conformers=1) - methane.generate_conformers(n_conformers=1) - - assert (methane_interchange.combine(ethane_interchange)).positions is None - methane_interchange.positions = methane.conformers[0] - assert (methane_interchange.combine(ethane_interchange)).positions is None - ethane_interchange.positions = ethane.conformers[0] - assert (methane_interchange.combine(ethane_interchange)).positions is not None - - @pytest.mark.parametrize("handler", ["Electrostatics", "vdW"]) - def test_error_mismatched_cutoffs( - self, - monkeypatch, - sage, - basic_top, - handler, - ): - - monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") - - sage_modified = ForceField("openff-2.1.0.offxml") - - sage_modified[handler].cutoff *= 1.5 - - with pytest.raises( - CutoffMismatchError, - match=f"{handler} cutoffs do not match", - ): - sage.create_interchange(basic_top).combine( - sage_modified.create_interchange(basic_top), - ) diff --git a/openff/interchange/_tests/unit_tests/operations/test_combine.py b/openff/interchange/_tests/unit_tests/operations/test_combine.py new file mode 100644 index 000000000..9cf8d5fbf --- /dev/null +++ b/openff/interchange/_tests/unit_tests/operations/test_combine.py @@ -0,0 +1,137 @@ +import copy + +import numpy +import pytest +from openff.toolkit import ForceField, Molecule, Topology, unit +from openff.utilities.testing import skip_if_missing + +from openff.interchange import Interchange +from openff.interchange.drivers import get_openmm_energies +from openff.interchange.exceptions import ( + CutoffMismatchError, + SwitchingFunctionMismatchError, +) + + +class TestCombine: + @skip_if_missing("openmm") + def test_basic_combination(self, monkeypatch, sage_unconstrained): + """Test basic use of Interchange.__add__() based on the README example""" + monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") + + mol = Molecule.from_smiles("C") + mol.generate_conformers(n_conformers=1) + top = Topology.from_molecules([mol]) + + interchange = Interchange.from_smirnoff(sage_unconstrained, top) + + interchange.box = [4, 4, 4] * numpy.eye(3) + interchange.positions = mol.conformers[0] + + # Copy and translate atoms by [1, 1, 1] + other = Interchange() + other = copy.deepcopy(interchange) + other.positions += 1.0 * unit.nanometer + + combined = interchange.combine(other) + + # Just see if it can be converted into OpenMM and run + get_openmm_energies(combined) + + def test_parameters_do_not_clash(self, monkeypatch, sage_unconstrained): + monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") + + thf = Molecule.from_smiles("C1CCOC1") + ace = Molecule.from_smiles("CC(=O)O") + + thf.generate_conformers(n_conformers=1) + ace.generate_conformers(n_conformers=1) + + def make_interchange(molecule: Molecule) -> Interchange: + molecule.generate_conformers(n_conformers=1) + interchange = Interchange.from_smirnoff( + force_field=sage_unconstrained, + topology=[molecule], + ) + interchange.positions = molecule.conformers[0] + + return interchange + + thf_interchange = make_interchange(thf) + ace_interchange = make_interchange(ace) + complex_interchange = thf_interchange.combine(ace_interchange) + + thf_vdw = thf_interchange["vdW"].get_system_parameters() + ace_vdw = ace_interchange["vdW"].get_system_parameters() + add_vdw = complex_interchange["vdW"].get_system_parameters() + + numpy.testing.assert_equal(numpy.vstack([thf_vdw, ace_vdw]), add_vdw) + + # TODO: Ensure the de-duplication is maintained after exports + + def test_positions_setting(self, monkeypatch, sage): + """Test that positions exist on the result if and only if + both input objects have positions.""" + + monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") + + ethane = Molecule.from_smiles("CC") + methane = Molecule.from_smiles("C") + + ethane_interchange = Interchange.from_smirnoff( + sage, + [ethane], + ) + methane_interchange = Interchange.from_smirnoff(sage, [methane]) + + ethane.generate_conformers(n_conformers=1) + methane.generate_conformers(n_conformers=1) + + assert (methane_interchange.combine(ethane_interchange)).positions is None + methane_interchange.positions = methane.conformers[0] + assert (methane_interchange.combine(ethane_interchange)).positions is None + ethane_interchange.positions = ethane.conformers[0] + assert (methane_interchange.combine(ethane_interchange)).positions is not None + + @pytest.mark.parametrize("handler", ["Electrostatics", "vdW"]) + def test_error_mismatched_cutoffs( + self, + monkeypatch, + sage, + basic_top, + handler, + ): + + monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") + + sage_modified = ForceField("openff-2.1.0.offxml") + + sage_modified[handler].cutoff *= 1.5 + + with pytest.raises( + CutoffMismatchError, + match=f"{handler} cutoffs do not match", + ): + sage.create_interchange(basic_top).combine( + sage_modified.create_interchange(basic_top), + ) + + def test_error_mismatched_switching_function( + self, + monkeypatch, + sage, + basic_top, + ): + + monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1") + + sage_modified = ForceField("openff-2.1.0.offxml") + + sage_modified["vdW"].switch_width *= 0.0 + + with pytest.raises( + SwitchingFunctionMismatchError, + ): + sage.create_interchange(basic_top).combine( + sage_modified.create_interchange(basic_top), + ) diff --git a/openff/interchange/exceptions.py b/openff/interchange/exceptions.py index a66645581..be0967d9d 100644 --- a/openff/interchange/exceptions.py +++ b/openff/interchange/exceptions.py @@ -106,6 +106,10 @@ class CutoffMismatchError(UnsupportedCombinationError): """Non-bonded cutoffs do not match.""" +class SwitchingFunctionMismatchError(UnsupportedCombinationError): + """Switching distances or the use of a switching function does not match.""" + + class PluginCompatibilityError(InterchangeException): """A plugin is incompatible with the current version of Interchange in the way it is called.""" diff --git a/openff/interchange/operations/_combine.py b/openff/interchange/operations/_combine.py index 62c088c60..14ea70159 100644 --- a/openff/interchange/operations/_combine.py +++ b/openff/interchange/operations/_combine.py @@ -9,6 +9,7 @@ from openff.interchange.components.toolkit import _combine_topologies from openff.interchange.exceptions import ( CutoffMismatchError, + SwitchingFunctionMismatchError, UnsupportedCombinationError, ) @@ -37,6 +38,12 @@ def _check_nonbonded_compatibility( f"{interchange1[key].cutoff} and {interchange2[key].cutoff}.", ) + if interchange1["vdW"].switch_width != interchange2["vdW"].switch_width: + raise SwitchingFunctionMismatchError( + f"Switching distance(s) do not match. Found " + f"{interchange1['vdW'].switch_width} and {interchange2['vdW'].switch_width}.", + ) + def _combine( input1: "Interchange", From 319626bd83574490c5f4b940570f64551c5aedea Mon Sep 17 00:00:00 2001 From: "Matthew W. Thompson" Date: Tue, 19 Mar 2024 12:46:24 -0500 Subject: [PATCH 5/5] DOC: Update release history --- docs/releasehistory.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/releasehistory.md b/docs/releasehistory.md index 8ced17496..6dd99b7f2 100644 --- a/docs/releasehistory.md +++ b/docs/releasehistory.md @@ -16,6 +16,7 @@ Please note that all releases prior to a version 1.0.0 are considered pre-releas * #933 Fixes #934 in which atom order was sometimes mangled in `Interchange.from_openmm`. * #929 A warning is raised when positions are not passed to `Interchange.from_openmm`. * #930 Adds `additional_forces` argument to `create_openmm_simulation`. +* #938 An error is raised when non-bonded settings do not match when using `Interchange.combine`. ## 0.3.23 - 2024-03-07