Support importing (some) virtual sites in Interchange.from_openmm

.github/workflows/ci.yaml

@@ -22,7 +22,6 @@ jobs:
     name: Test on ${{ matrix.os }}, Python ${{ matrix.python-version }}, OpenMM ${{ matrix.openmm }}, Pydantic ${{ matrix.pydantic-version }}, OpenEye ${{ matrix.openeye }}
     runs-on: ${{ matrix.os }}
     strategy:
-      fail-fast: false
       matrix:
         os:
           - macos-latest

devtools/conda-envs/test_env.yaml

@@ -17,8 +17,8 @@ dependencies:
   - openmm =8.1.2
   # smirnoff-plugins =2024
   # de-forcefields  # add back after smirnoff-plugins update
-  - openff-nagl
-  - openff-nagl-models
+  - openff-nagl =0.5
+  - openff-nagl-models =0.3
   - mbuild ~=0.18
   - foyer =1
   - gmso ~=0.12

docs/releasehistory.md

@@ -15,6 +15,7 @@ Please note that all releases prior to a version 1.0.0 are considered pre-releas
 
 ### New features
 
+* #1081 `Interchange.from_openmm` now processes virtual sites, but only `openmm.ThreeParticleAverageSite`s.
 * #1053 Logs, at the level of `logging.INFO`, how charges are assigned by SMIRNOFF force fields to each atom and virtual site.
 * #1080 HMR is supported with OpenMM when virtual sites are present.
 

docs/using/edges.md

@@ -67,6 +67,19 @@ For more, see [issue #1005](https://github.com/openforcefield/openff-interchange
 
 Keywords: OpenMM, GROMACS, constraints, bond constraints, rigid water
 
+### Virtual site exclusions re-created with "parents" virtual site exclusion policy
+
+Non-bonded exclusions involving virtual sites (between virtual sites and heavy atoms or between
+virtual sites and virtual sites) are not processed. Instead, they are later re-generated assuming the "parents" exclusion policy as defined in the [SMIRNOFF specification](https://openforcefield.github.io/standards/standards/smirnoff/#virtualsites-virtual-sites-for-off-atom-charges). This should re-create typical exclusions in 4- and 5-site water models but may not be appropriate with highly custom virtual site interactions in larger molecules.
+
+### Virtual sites from multiple sources cannot be mixed
+
+Combining systems with virtual sites from multiple sources is not fully-featured. For example, this refers to importing a box of TIP4P-containing solvent from OpenMM with a ligand prepared with SMIRNOFF virtual sites parameters.
+
+### Virtual sites must be listed after heavy atoms each molecule
+
+It's assumed that, in each molecule in an OpenMM topology, all heavy atoms are listed before any virtual sites. This includes the case of all virtual sites being listed after all heavy atoms, i.e. not collated into molecules/residues. There are no community standards areound particle ordering, but virtual sites are typically listed after heavy atoms in each molecule or residue.
+
 ## Quirks with GROMACS
 
 ### Residue indices must begin at 1

openff/interchange/_tests/conftest.py

@@ -13,21 +13,28 @@
 
 
 @pytest.fixture
-def sage():
+def sage() -> ForceField:
     return ForceField("openff-2.0.0.offxml")
 
 
 @pytest.fixture
-def sage_unconstrained():
+def sage_unconstrained() -> ForceField:
     return ForceField("openff_unconstrained-2.0.0.offxml")
 
 
 @pytest.fixture
-def sage_no_switch(sage):
+def sage_no_switch(sage) -> ForceField:
     sage["vdW"].switch_width = Quantity(0.0, "angstrom")
     return sage
 
 
+@pytest.fixture
+def sage_with_tip4p() -> ForceField:
+    # re-build off of existing fixtures if this gets implemented
+    # https://github.com/openforcefield/openff-toolkit/issues/1948
+    return ForceField("openff-2.0.0.offxml", "tip4p_fb.offxml")
+
+
 @pytest.fixture
 def sage_with_bond_charge(sage):
     sage["Bonds"].add_parameter(

openff/interchange/_tests/interoperability_tests/components/interchange/test_combine.py

@@ -7,7 +7,6 @@
 
 
 def test_combine_after_from_openmm_with_mainline_openmm_force_field(
-    monkeypatch,
     popc,
     sage,
 ):
@@ -17,8 +16,6 @@ def test_combine_after_from_openmm_with_mainline_openmm_force_field(
     import openmm.app
     import openmm.unit
 
-    monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
     ligand = MoleculeWithConformer.from_smiles("c1ccccc1")
     ligand._conformers[0] += Quantity([3, 3, 3], "angstrom")
     topology = ligand.to_topology()

openff/interchange/_tests/test_issues.py

@@ -78,11 +78,9 @@ def test_issue_1022(pack):
 
 
 @skip_if_missing("openmm")
-def test_issue_1031(monkeypatch):
+def test_issue_1031():
     import openmm.app
 
-    monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
     # just grab some small PDB file from the toolkit, doesn't need to be huge, just
     # needs to include some relevant atom names
     openmm_topology = openmm.app.PDBFile(

openff/interchange/_tests/unit_tests/components/test_interchange.py

@@ -411,9 +411,7 @@ def test_from_gromacs_error(self):
             Interchange.from_gromacs()
 
     @skip_if_missing("openmm")
-    def test_from_openmm_called(self, monkeypatch, simple_interchange):
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
+    def test_from_openmm_called(self, simple_interchange):
         topology = simple_interchange.to_openmm_topology()
         system = simple_interchange.to_openmm()
         positions = simple_interchange.positions
@@ -435,7 +433,7 @@ def test_to_amber(self, simple_interchange):
             shell=True,
         )
 
-    def test_from_gromacs_called(self, monkeypatch, simple_interchange):
+    def test_from_gromacs_called(self, simple_interchange, monkeypatch):
         monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
 
         simple_interchange.to_gromacs(prefix="tmp_")

openff/interchange/_tests/unit_tests/interop/openmm/_import/test_compat.py

@@ -12,9 +12,7 @@
 
 class TestUnsupportedCases:
     @pytest.mark.filterwarnings("ignore:.*are you sure you don't want to pass positions")
-    def test_error_topology_mismatch(self, monkeypatch, sage_unconstrained, ethanol):
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
+    def test_error_topology_mismatch(self, sage_unconstrained, ethanol):
         topology = ethanol.to_topology()
         topology.box_vectors = Quantity([4, 4, 4], "nanometer")
 
@@ -38,26 +36,60 @@ def test_error_topology_mismatch(self, monkeypatch, sage_unconstrained, ethanol)
                 topology=other_topology.to_openmm(),
             )
 
-    def test_found_virtual_sites(self, monkeypatch, tip4p, water):
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
+    def test_found_out_of_plane_virtual_site(self, water_dimer):
+        pytest.importorskip("openmm")
 
-        topology = water.to_topology()
-        topology.box_vectors = Quantity([4, 4, 4], "nanometer")
+        import openmm.app
+
+        modeller = openmm.app.Modeller(
+            topology=water_dimer.to_openmm(),
+            positions=water_dimer.get_positions().to("nanometer").to_openmm(),
+        )
 
-        system = tip4p.create_openmm_system(topology)
+        forcefield = openmm.app.ForceField("tip5p.xml")
+
+        modeller.addExtraParticles(forcefield=forcefield)
+
+        system = forcefield.createSystem(
+            modeller.topology,
+            nonbondedMethod=openmm.app.PME,
+            nonbondedCutoff=1.0 * openmm.unit.nanometers,
+            constraints=openmm.app.HBonds,
+            rigidWater=True,
+            ewaldErrorTolerance=0.0005,
+        )
 
         with pytest.raises(
             UnsupportedImportError,
-            match="A particle is a virtual site, which is not yet supported.",
+            match="A particle is a virtual site of type.*OutOfPlane.*which is not yet supported.",
         ):
             from_openmm(
                 system=system,
-                topology=topology.to_openmm(),
+                topology=modeller.topology,
             )
 
-    def test_missing_positions_warning(self, monkeypatch, sage, water):
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
+    @pytest.mark.skip(
+        reason="Need to find a way to get OpenMM to actually use TwoParticleAverageSite",
+    )
+    def test_found_two_particle_average_virtual_site(
+        self,
+        sage_with_bond_charge,
+        default_integrator,
+    ):
+        simulation = sage_with_bond_charge.create_interchange(
+            Molecule.from_smiles("CCl").to_topology(),
+        ).to_openmm_simulation(integrator=default_integrator)
+
+        with pytest.raises(
+            UnsupportedImportError,
+            match="A particle is a `TwoParticleAverage` virtual site, which is not yet supported.",
+        ):
+            from_openmm(
+                system=simulation.system,
+                topology=simulation.topology,
+            )
 
+    def test_missing_positions_warning(self, sage, water):
         topology = water.to_topology()
         topology.box_vectors = Quantity([4, 4, 4], "nanometer")
 

openff/interchange/_tests/unit_tests/interop/openmm/_import/test_import.py

@@ -1,5 +1,6 @@
 import copy
 import random
+from collections import defaultdict
 
 import numpy
 import pytest
@@ -20,9 +21,7 @@
 
 @skip_if_missing("openmm")
 class TestFromOpenMM:
-    def test_simple_roundtrip(self, monkeypatch, sage_unconstrained, ethanol):
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
+    def test_simple_roundtrip(self, sage_unconstrained, ethanol):
         ethanol.generate_conformers(n_conformers=1)
 
         interchange = Interchange.from_smirnoff(
@@ -69,13 +68,12 @@ def simple_system(self):
     @pytest.mark.parametrize("as_argument", [False, True])
     def test_different_ways_to_process_box_vectors(
         self,
-        monkeypatch,
         as_argument,
         simple_system,
     ):
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
         topology = Molecule.from_smiles("C").to_topology()
+        topology._molecule_virtual_site_map = defaultdict(list)
+        topology._particle_map = {index: index for index in range(topology.n_atoms)}
 
         if as_argument:
             box = Interchange.from_openmm(
@@ -100,8 +98,6 @@ def test_topology_and_system_box_vectors_differ(
         simple_system,
     ):
         """Ensure that, if box vectors specified in the topology and system differ, those in the topology are used."""
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
         topology = Molecule.from_smiles("C").to_topology()
         topology.box_vectors = Quantity([4, 5, 6], unit.nanometer)
 
@@ -112,9 +108,7 @@ def test_topology_and_system_box_vectors_differ(
 
         assert numpy.diag(box.m_as(unit.nanometer)) == pytest.approx([4, 5, 6])
 
-    def test_openmm_roundtrip_metadata(self, monkeypatch, sage):
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
+    def test_openmm_roundtrip_metadata(self, sage):
         # Make an example OpenMM Topology with metadata.
         # Here we use OFFTK to make the OpenMM Topology, but this could just as easily come from another source
         ethanol = Molecule.from_smiles("CCO")
@@ -155,12 +149,10 @@ def test_openmm_roundtrip_metadata(self, monkeypatch, sage):
             assert atom.metadata["residue_name"] == "BNZ"
 
     @pytest.mark.slow
-    def test_openmm_native_roundtrip_metadata(self, monkeypatch, sage):
+    def test_openmm_native_roundtrip_metadata(self, sage):
         """
         Test that metadata is the same whether we load a PDB through OpenMM+Interchange vs. Topology.from_pdb.
         """
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
         pdb = openmm.app.PDBFile(
             get_data_file_path(
                 "ALA_GLY/ALA_GLY.pdb",
@@ -184,15 +176,13 @@ def test_openmm_native_roundtrip_metadata(self, monkeypatch, sage):
             del off_atom_metadata["match_info"]
             assert roundtrip_atom.metadata == off_atom_metadata
 
-    def test_electrostatics_cutoff_not_ignored(self, monkeypatch, ethanol):
+    def test_electrostatics_cutoff_not_ignored(self, ethanol):
         pytest.importorskip("openmmforcefields")
 
         import openmm.app
         import openmm.unit
         from openmmforcefields.generators import GAFFTemplateGenerator
 
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
         topology = ethanol.to_topology()
         topology.box_vectors = Quantity([4, 4, 4], "nanometer")
 
@@ -218,13 +208,12 @@ def test_electrostatics_cutoff_not_ignored(self, monkeypatch, ethanol):
         assert interchange["vdW"].cutoff.m_as(unit.nanometer) == pytest.approx(1.2345)
 
     @needs_gmx
-    def test_fill_in_rigid_water_parameters(self, water_dimer, monkeypatch):
+    @pytest.mark.skip(reason="needs OpenMM -> Interchange -> GROMACS virtual sites implemented")
+    def test_fill_in_rigid_water_parameters(self, water_dimer):
         import openmm.app
 
         from openff.interchange.drivers import get_gromacs_energies
 
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
         openmm_force_field = openmm.app.ForceField("tip3p.xml")
         openmm_topology = water_dimer.to_openmm()
 
@@ -252,11 +241,12 @@ def test_fill_in_rigid_water_parameters(self, water_dimer, monkeypatch):
 
 @skip_if_missing("openmm")
 class TestProcessTopology:
-    def test_with_openff_topology(self, monkeypatch, sage, basic_top):
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
+    def test_with_openff_topology(self, sage, basic_top):
         system = sage.create_openmm_system(basic_top)
 
+        basic_top._molecule_virtual_site_map = defaultdict(list)
+        basic_top._particle_map = {index: index for index in range(basic_top.n_atoms)}
+
         with_openff = Interchange.from_openmm(
             system=system,
             topology=basic_top,
@@ -298,7 +288,7 @@ def test_unsupported_method(self):
             force.setNonbondedMethod(method)
 
             with pytest.raises(UnsupportedImportError):
-                _convert_nonbonded_force(force)
+                _convert_nonbonded_force(force, dict())
 
     def test_parse_switching_distance(self):
         force = openmm.NonbondedForce()
@@ -311,7 +301,7 @@ def test_parse_switching_distance(self):
         force.setUseSwitchingFunction(True)
         force.setSwitchingDistance(cutoff - switch_width)
 
-        vdw, _ = _convert_nonbonded_force(force)
+        vdw, _ = _convert_nonbonded_force(force=force, particle_map=dict())
 
         assert vdw.cutoff.m_as(unit.nanometer) == pytest.approx(cutoff)
         assert vdw.switch_width.m_as(unit.nanometer) == pytest.approx(switch_width)
@@ -324,18 +314,16 @@ def test_parse_switching_distance_unused(self):
 
         force.setCutoffDistance(cutoff)
 
-        vdw, _ = _convert_nonbonded_force(force)
+        vdw, _ = _convert_nonbonded_force(force=force, particle_map=dict())
 
         assert vdw.cutoff.m_as(unit.nanometer) == pytest.approx(cutoff)
         assert vdw.switch_width.m_as(unit.nanometer) == 0.0
 
 
 @skip_if_missing("openmm")
 class TestConvertConstraints:
-    def test_num_constraints(self, monkeypatch, sage, basic_top):
+    def test_num_constraints(self, sage, basic_top):
         """Test that the number of constraints is preserved when converting to and from OpenMM"""
-        monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")
-
         interchange = sage.create_interchange(basic_top)
 
         converted = Interchange.from_openmm(