diff --git a/.github/workflows/examples.yaml b/.github/workflows/examples.yaml index d0f634b1d..a0dc976f5 100644 --- a/.github/workflows/examples.yaml +++ b/.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 diff --git a/devtools/conda-envs/examples_env.yaml b/devtools/conda-envs/examples_env.yaml index 8abb88ef0..74b83f3e2 100644 --- a/devtools/conda-envs/examples_env.yaml +++ b/devtools/conda-envs/examples_env.yaml @@ -30,6 +30,7 @@ dependencies: - mdtraj - pytest - pytest-xdist + - pytest-cov - nbval # Examples - openmmforcefields diff --git a/docs/using/edges.md b/docs/using/edges.md index 74898ed15..6a86e9ef3 100644 --- a/docs/using/edges.md +++ b/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). diff --git a/openff/interchange/_tests/unit_tests/operations/test_combine.py b/openff/interchange/_tests/unit_tests/operations/test_combine.py index 98cf27a00..ad5d63031 100644 --- a/openff/interchange/_tests/unit_tests/operations/test_combine.py +++ b/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()), + ) diff --git a/openff/interchange/operations/_combine.py b/openff/interchange/operations/_combine.py index d55b57a1f..000439d6f 100644 --- a/openff/interchange/operations/_combine.py +++ b/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",