Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve testing of GROMACS import #1118

Merged
merged 3 commits into from
Jan 15, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions openff/interchange/_tests/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,9 @@ class MoleculeWithConformer(Molecule):
"""Thin wrapper around `Molecule` to produce an instance with a conformer in one call."""

@classmethod
def from_smiles(self, smiles, name="", **kwargs):
def from_smiles(self, smiles, name="", *args, **kwargs):
"""Create from smiles and generate a single conformer."""
molecule = super().from_smiles(smiles, **kwargs)
molecule = super().from_smiles(smiles, *args, **kwargs)
mattwthompson marked this conversation as resolved.
Show resolved Hide resolved
molecule.generate_conformers(n_conformers=1)
molecule.name = name

Expand Down
14 changes: 12 additions & 2 deletions openff/interchange/_tests/_gromacs.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,24 @@
from openff.utilities import temporary_cd

from openff.interchange import Interchange
from openff.interchange.components.mdconfig import get_smirnoff_defaults
from openff.interchange.drivers import get_gromacs_energies


def gmx_monolithic_vs_itp(state: Interchange):
def gmx_roundtrip(state: Interchange, apply_smirnoff_defaults: bool = False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not blocking) My understanding of this method is that it just ensures that an error isn't raised when writing and then reading the system in GROMACS formats. It doesn't evaluate anything numerical or return the roundtripped system for further inspection. This was a little confusing to me, it might be good to update the method name or docstring to make that clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find, I think that was incidentally removed in a merge

with temporary_cd():
get_gromacs_energies(state, _monolithic=True).compare(get_gromacs_energies(state, _monolithic=False))
state.to_gromacs(prefix="state", decimal=8)
new_state = Interchange.from_gromacs(topology_file="state.top", gro_file="state.gro")

get_smirnoff_defaults(periodic=True).apply(new_state)

get_gromacs_energies(state).compare(get_gromacs_energies(new_state))


def gmx_monolithic_vs_itp(state: Interchange):
with temporary_cd():
# TODO: More helpful handling of failures, i.e.
# * Detect differences in positions
# * Detect differences in box vectors
# * Detect differences in non-bonded settings
get_gromacs_energies(state, _monolithic=True).compare(get_gromacs_energies(state, _monolithic=False))
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
"""Interoperability tests with GROMACS."""
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from openff.toolkit import Quantity

from openff.interchange._tests._gromacs import gmx_monolithic_vs_itp
from openff.interchange._tests._gromacs import gmx_monolithic_vs_itp, gmx_roundtrip


def test_ligand_vacuum(caffeine, sage_unconstrained, monkeypatch):
Expand All @@ -9,16 +9,19 @@ def test_ligand_vacuum(caffeine, sage_unconstrained, monkeypatch):
topology = caffeine.to_topology()
topology.box_vectors = Quantity([4, 4, 4], "nanometer")

gmx_roundtrip(sage_unconstrained.create_interchange(topology))
gmx_monolithic_vs_itp(sage_unconstrained.create_interchange(topology))


def test_water_dimer(water_dimer, tip3p, monkeypatch):
monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")

gmx_roundtrip(tip3p.create_interchange(water_dimer))
gmx_monolithic_vs_itp(tip3p.create_interchange(water_dimer))


def test_alanine_dipeptide(alanine_dipeptide, ff14sb, monkeypatch):
monkeypatch.setenv("INTERCHANGE_EXPERIMENTAL", "1")

gmx_roundtrip(ff14sb.create_interchange(alanine_dipeptide))
gmx_monolithic_vs_itp(ff14sb.create_interchange(alanine_dipeptide))
4 changes: 0 additions & 4 deletions openff/interchange/_tests/unit_tests/drivers/test_openmm.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,6 @@ class TestReportWithPlugins:
pytest.importorskip("smirnoff_plugins")
pytest.importorskip("openeye")

@pytest.fixture
def ligand(self):
return MoleculeWithConformer.from_smiles("CC[C@@](/C=C\\[H])(C=C)O")

@pytest.fixture
def de_force_field(self) -> ForceField:
return ForceField(
Expand Down
5 changes: 4 additions & 1 deletion openff/interchange/components/mdconfig.py
Original file line number Diff line number Diff line change
Expand Up @@ -462,12 +462,15 @@ def _infer_constraints(interchange: "Interchange") -> str:

if num_constraints == num_h_bonds:
return "h-bonds"
elif num_constraints == len(interchange["Bonds"].key_map):
elif num_constraints == num_bonds:
return "all-bonds"
elif num_constraints == (num_bonds + num_angles):
return "all-angles"

else:
# TODO: Rigid waters may not have bond and angle parameters, but still have 3 constraints
# per molecule. There should be a better way to process these, but it's non-trivial
# to detect water molecules in a performance, scalable way without false positives.
warnings.warn(
"Ambiguous failure while processing constraints. Constraining h-bonds as a stopgap.",
)
Expand Down
2 changes: 2 additions & 0 deletions openff/interchange/drivers/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
from openff.interchange.drivers.gromacs import get_gromacs_energies
from openff.interchange.drivers.lammps import get_lammps_energies
from openff.interchange.drivers.openmm import get_openmm_energies
from openff.interchange.drivers.report import EnergyReport

__all__ = [
"EnergyReport",
"get_all_energies",
"get_amber_energies",
"get_gromacs_energies",
Expand Down
Loading