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

Create blank collections when importing only water #1134

Merged
merged 1 commit into from
Jan 10, 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
1 change: 1 addition & 0 deletions docs/releasehistory.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ Please note that all releases prior to a version 1.0.0 are considered pre-releas

* #1070 The `charge_from_molecules` argument must include only molecules that contain partial charges and are non-isomorphic with each other.
* #1070 The `charge_from_molecules` argument as used by the OpenFF Toolkit is handled internally as `molecules_with_preset_charges`.
* #1134 Fixes a bug in which importing OpenMM systems containing only rigid water would crash.

### Performance improvements

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@
import random

import numpy
import openmm
import pytest
from openff.toolkit import Molecule, Quantity, Topology, unit
from openff.toolkit import ForceField, Molecule, Quantity, Topology, unit
from openff.utilities import get_data_file_path, has_package, skip_if_missing

from openff.interchange import Interchange
Expand Down Expand Up @@ -249,6 +250,33 @@ def test_fill_in_rigid_water_parameters(self, water_dimer, monkeypatch):
},
)

def test_only_constrained_water(self, water_dimer):
water_dimer.box_vectors = Quantity([4, 4, 4], "nanometer")

interchange = ForceField("openff-2.2.1.offxml").create_interchange(water_dimer)

simulation = interchange.to_openmm_simulation(integrator=openmm.LangevinIntegrator(300, 1, 0.001))
system = simulation.system

for index in range(system.getNumForces()):
if isinstance(system.getForce(index), openmm.HarmonicBondForce):
break

system.removeForce(index)

for index in range(system.getNumForces()):
if isinstance(system.getForce(index), openmm.HarmonicAngleForce):
break

system.removeForce(index)

interchange2 = Interchange.from_openmm(
system=simulation.system,
topology=simulation.topology,
)

assert interchange2.topology.n_bonds == interchange.topology.n_bonds


@skip_if_missing("openmm")
class TestProcessTopology:
Expand Down
13 changes: 12 additions & 1 deletion openff/interchange/interop/openmm/_import/_import.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,12 @@ def from_openmm(
# TODO: Does this run through the Interchange.box validator?
interchange.box = _box_vectors

if interchange.topology.n_bonds > len(interchange.collections["Bonds"].key_map):
try:
num_physics_bonds = len(interchange["Bonds"].key_map)
except LookupError:
num_physics_bonds = 0

if interchange.topology.n_bonds > num_physics_bonds:
# There are probably missing (physics) bonds from rigid waters. The topological
# bonds are probably processed correctly.
_fill_in_rigid_water_bonds(interchange)
Expand Down Expand Up @@ -348,6 +353,12 @@ def _fill_in_rigid_water_bonds(interchange: "Interchange"):
from openff.interchange.components.potentials import Potential
from openff.interchange.models import AngleKey, BondKey, PotentialKey

if "Bonds" not in interchange.collections:
interchange.collections.update({"Bonds": BondCollection()})

if "Angles" not in interchange.collections:
interchange.collections.update({"Angles": AngleCollection()})

Copy link
Contributor

Choose a reason for hiding this comment

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

In my personal opinion it is a bit more logical to create those empty collections earlier, when all the collections are created. Those collections are anyway created, since _fill_in_rigid_water_bonds is anyway called, but now, _fill_in_rigid_water_bonds has sort of two tasks, which are not logically connected.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think of this as a sort of special case since it's only hit when the entire system lacks bonds and the collections are created elsewhere in dedicated private methods. I agree it shouldn't make a difference

The only problematic case I can think of is a system that constrained angles but not constrained bonds, which is weird and certainly outside what I think of as a normal treatment of rigid water

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see you point. I think the PR is good. Thanks a lot for a prompt fix.

simple_water = _SimpleMolecule.from_molecule(Molecule.from_smiles("O"))

rigid_water_bond_key = PotentialKey(id="rigid_water", associated_handler="Bonds")
Expand Down
Loading