Merge pull request #1153 from openforcefield/error-combine-mixed-14

Error when non-bonded 1-4 scaling factors differ significantly

.github/workflows/examples.yaml

@@ -55,10 +55,10 @@ jobs:
         SECRET_OE_LICENSE: ${{ secrets.OE_LICENSE }}
 
     - name: Run docexamples
-      run: python -m pytest -c /dev/null/ --doctest-modules openff/interchange/ --ignore=openff/interchange/_tests
+      run: python -m pytest -c pyproject.toml --doctest-modules openff/interchange/ --ignore=openff/interchange/_tests
 
     - name: Run example notebooks
       run: |
-        python -m pytest -c /dev/null/ --nbval-lax --dist loadscope -n logical --durations=20 examples/ \
+        python -m pytest -c pyproject.toml --nbval-lax --dist loadscope -n logical --durations=20 examples/ \
           --ignore=examples/deprecated/ \
           --ignore=examples/experimental

devtools/conda-envs/examples_env.yaml

@@ -30,6 +30,7 @@ dependencies:
   - mdtraj
   - pytest
   - pytest-xdist
+  - pytest-cov
   - nbval
   # Examples
   - openmmforcefields

docs/using/edges.md

@@ -47,6 +47,10 @@ For example, `Interchange.topology.get_positions()` will never include positions
 
 ## Quirks of `Interchange.combine`
 
+### Electrostatics 1-4 scaling factors may be slightly modified
+
+Amber family force fields historically use a 1-4 scaling factor of 1 / 1.2 (or 5/6). Some older OpenFF force field releases round this to 6 digits (0.833333) but more recent releases round this to 10 digits (0.8333333333). These are not strictly equal and arguably should not be combined, but (in this case only) for easier compatibility this difference is ignored and combination proceeds as if they were both 0.833333333 from the start. If value differ significantly, i.e. 0.5 vs 0.833333, an error is raised as this difference is non-trivial.
+
 ### Charges of isomorphic molecules may be overwritten
 
 When isomorphic molecules are found on the `Interchange` objects used in `.combine`, the charges of molecules in the argument object are used. For example, if objects `interchange1` and `interchange2` each contain toluene molecules with different partial charges, the charges from `interchange2` will be used on the object returned by `interchange1.combine(interchange2)`. For more, see [Issue #1075](https://github.com/openforcefield/openff-interchange/issues/1075).

openff/interchange/_tests/unit_tests/operations/test_combine.py

@@ -6,11 +6,14 @@
 from openff.utilities.testing import skip_if_missing
 
 from openff.interchange import Interchange
+from openff.interchange._tests import MoleculeWithConformer
 from openff.interchange.drivers import get_openmm_energies
 from openff.interchange.exceptions import (
     CutoffMismatchError,
     SwitchingFunctionMismatchError,
+    UnsupportedCombinationError,
 )
+from openff.interchange.warnings import InterchangeCombinationWarning
 
 
 class TestCombine:
@@ -19,8 +22,7 @@ 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)
+        mol = MoleculeWithConformer.from_smiles("C")
         top = Topology.from_molecules([mol])
 
         interchange = Interchange.from_smirnoff(sage_unconstrained, top)
@@ -43,14 +45,10 @@ def test_basic_combination(self, monkeypatch, sage_unconstrained):
     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 = MoleculeWithConformer.from_smiles("C1CCOC1")
+        ace = MoleculeWithConformer.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)
+        def make_interchange(molecule: MoleculeWithConformer) -> Interchange:
             interchange = Interchange.from_smirnoff(
                 force_field=sage_unconstrained,
                 topology=[molecule],
@@ -135,3 +133,50 @@ def test_error_mismatched_switching_function(
             sage.create_interchange(basic_top).combine(
                 sage_modified.create_interchange(basic_top),
             )
+
+    @pytest.mark.parametrize(
+        argnames=["vdw", "electrostatics"],
+        argvalues=[
+            (True, False),
+            (False, True),
+            (True, True),
+            (False, False),
+        ],
+    )
+    def test_dont_combine_mixed_14(self, sage, vdw, electrostatics):
+        """
+        Until it's implemented, error out when any non-bonded collections have non-equal 1-4 scaling factors.
+
+        See https://github.com/openforcefield/openff-interchange/issues/380
+
+        """
+        interchange1 = sage.create_interchange(MoleculeWithConformer.from_smiles("C").to_topology())
+        interchange2 = sage.create_interchange(MoleculeWithConformer.from_smiles("CCO").to_topology())
+
+        if vdw:
+            interchange2["vdW"].scale_14 = 0.444
+
+        if electrostatics:
+            interchange2["Electrostatics"].scale_14 = 0.444
+
+        if vdw or electrostatics:
+            with pytest.raises(
+                UnsupportedCombinationError,
+                match="1-4.*vdW" if vdw else "1-4.*Electro",
+            ):
+                interchange2.combine(interchange1)
+        else:
+            # if neither are modified, that error shouldn't be raised
+            interchange2.combine(interchange1)
+
+    def test_mix_different_5_6_rounding(self, parsley, sage, ethanol):
+        """Test that 0.833333 is 'upconverted' to 0.8333333333 in combination."""
+        with pytest.warns(
+            InterchangeCombinationWarning,
+            match="more digits in rounding",
+        ):
+            parsley.create_interchange(
+                ethanol.to_topology(),
+            ).combine(
+                sage.create_interchange(ethanol.to_topology()),
+            )

openff/interchange/operations/_combine.py

@@ -44,6 +44,31 @@ def _check_nonbonded_compatibility(
             f"{interchange1['vdW'].switch_width} and {interchange2['vdW'].switch_width}.",
         )
 
+    if interchange1["vdW"].scale_14 != interchange2["vdW"].scale_14:
+        raise UnsupportedCombinationError(
+            "1-4 scaling factors in vdW handler(s) do not match.",
+        )
+
+    if interchange1["Electrostatics"].scale_14 != interchange2["Electrostatics"].scale_14:
+        if sorted([interchange1["Electrostatics"].scale_14, interchange2["Electrostatics"].scale_14]) == [
+            0.833333,
+            0.8333333333,
+        ]:
+            warnings.warn(
+                "Found electrostatics 1-4 scaling factors of 5/6 with slightly different rounding "
+                "(0.833333 and 0.8333333333). This likely stems from OpenFF using more digits in rounding 1/1.2. "
+                "The value of 0.8333333333 will be used, which may or may not introduce small errors. ",
+                InterchangeCombinationWarning,
+            )
+
+            interchange1["Electrostatics"].scale_14 = 0.8333333333
+            interchange2["Electrostatics"].scale_14 = 0.8333333333
+
+        else:
+            raise UnsupportedCombinationError(
+                "1-4 scaling factors in Electrostatics handler(s) do not match.",
+            )
+
 
 def _combine(
     input1: "Interchange",