From 1ce3278591d13d5f4168977f7aaab31af22c2286 Mon Sep 17 00:00:00 2001 From: "Matthew W. Thompson" Date: Fri, 24 Jan 2025 17:07:51 -0600 Subject: [PATCH 1/4] TST: Add test for combining mixed 1-4 vdW and/or electrostatics --- .../unit_tests/operations/test_combine.py | 47 +++++++++++++++---- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/openff/interchange/_tests/unit_tests/operations/test_combine.py b/openff/interchange/_tests/unit_tests/operations/test_combine.py index 98cf27a00..1ea4e7c7e 100644 --- a/openff/interchange/_tests/unit_tests/operations/test_combine.py +++ b/openff/interchange/_tests/unit_tests/operations/test_combine.py @@ -6,10 +6,12 @@ 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, ) @@ -19,8 +21,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 +44,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 +132,35 @@ 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.*nonbond"): + interchange2.combine(interchange1) + else: + # if neither are modified, that error shouldn't be raised + interchange2.combine(interchange1) From dae1716d5e729b3173b801ee55dc2276546a2e9d Mon Sep 17 00:00:00 2001 From: "Matthew W. Thompson" Date: Fri, 24 Jan 2025 17:09:00 -0600 Subject: [PATCH 2/4] ENH: Error when 1-4 scaling factors don't match --- openff/interchange/operations/_combine.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/openff/interchange/operations/_combine.py b/openff/interchange/operations/_combine.py index d55b57a1f..9ed1f792f 100644 --- a/openff/interchange/operations/_combine.py +++ b/openff/interchange/operations/_combine.py @@ -44,6 +44,13 @@ def _check_nonbonded_compatibility( f"{interchange1['vdW'].switch_width} and {interchange2['vdW'].switch_width}.", ) + if (interchange1["vdW"].scale_14 != interchange2["vdW"].scale_14) or ( + interchange1["Electrostatics"].scale_14 != interchange2["Electrostatics"].scale_14 + ): + raise UnsupportedCombinationError( + "1-4 scaling factors in nonbonded handler(s) (vdW and/or Electrostatics do not match.", + ) + def _combine( input1: "Interchange", From 5be8691235573a015f72d22a004ba1e6e136da8a Mon Sep 17 00:00:00 2001 From: "Matthew W. Thompson" Date: Thu, 30 Jan 2025 10:28:43 -0600 Subject: [PATCH 3/4] FIX: Up-convert 0.833333 to 0.8333333333 --- .github/workflows/examples.yaml | 4 +-- devtools/conda-envs/examples_env.yaml | 1 + docs/using/edges.md | 6 +++++ .../unit_tests/operations/test_combine.py | 18 ++++++++++++- openff/interchange/operations/_combine.py | 26 ++++++++++++++++--- 5 files changed, 48 insertions(+), 7 deletions(-) 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 bf77d7696..0db220ba7 100644 --- a/docs/using/edges.md +++ b/docs/using/edges.md @@ -45,6 +45,12 @@ For example, `Interchange.topology.get_positions()` will never include positions `Molecule` and `Topology` objects can store partial charges, but these are ignored by default in methods like `Interchange.from_smirnoff`. This is because partial charges in SMIRNOFF force fields are defined by sections in the force field. To override this behavior, use the `charge_from_molecules` argument. Be aware that charges, and as a result most physics, will differ from what's perscribed by the contents of the force field. +## 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 stricly 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. + ## Quirks of `from_openmm` ### Modified masses are ignored diff --git a/openff/interchange/_tests/unit_tests/operations/test_combine.py b/openff/interchange/_tests/unit_tests/operations/test_combine.py index 1ea4e7c7e..ad5d63031 100644 --- a/openff/interchange/_tests/unit_tests/operations/test_combine.py +++ b/openff/interchange/_tests/unit_tests/operations/test_combine.py @@ -13,6 +13,7 @@ SwitchingFunctionMismatchError, UnsupportedCombinationError, ) +from openff.interchange.warnings import InterchangeCombinationWarning class TestCombine: @@ -159,8 +160,23 @@ def test_dont_combine_mixed_14(self, sage, vdw, electrostatics): interchange2["Electrostatics"].scale_14 = 0.444 if vdw or electrostatics: - with pytest.raises(UnsupportedCombinationError, match="1-4.*nonbond"): + 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 9ed1f792f..000439d6f 100644 --- a/openff/interchange/operations/_combine.py +++ b/openff/interchange/operations/_combine.py @@ -44,13 +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) or ( - interchange1["Electrostatics"].scale_14 != interchange2["Electrostatics"].scale_14 - ): + if interchange1["vdW"].scale_14 != interchange2["vdW"].scale_14: raise UnsupportedCombinationError( - "1-4 scaling factors in nonbonded handler(s) (vdW and/or Electrostatics do not match.", + "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", From b5ee55085eaebfd5476a5f42a9e1c63c83a62b21 Mon Sep 17 00:00:00 2001 From: Matt Thompson Date: Wed, 5 Feb 2025 16:57:54 -0600 Subject: [PATCH 4/4] Update docs/using/edges.md Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com> --- docs/using/edges.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/using/edges.md b/docs/using/edges.md index 0db220ba7..54fefaaaa 100644 --- a/docs/using/edges.md +++ b/docs/using/edges.md @@ -49,7 +49,7 @@ For example, `Interchange.topology.get_positions()` will never include positions ### 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 stricly 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. +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. ## Quirks of `from_openmm`