diff --git a/docs/using/edges.md b/docs/using/edges.md index 06211d4a..33eeeb69 100644 --- a/docs/using/edges.md +++ b/docs/using/edges.md @@ -1,5 +1,16 @@ # Sharp edges +## Quirks of core OpenFF objects + +Future refactors may remove the side effects of these quirks, but currently there are some +surprising inconsistencies in some API points between different OpenFF tools. + +### Contents of `Interchange.topology` and `Interchange` may not always be in sync + +Currently, the `Interchange.topology` attribute is defined by the OpenFF Toolkit's `Topology` object, which is feature-rich in cheminformatics functionality but not designed for MM interoperability. Most importantly, that representation does not include virtual sites (because molecules do not have virtual sites/dummy atoms). As a result, functionality involving virtual sites must go through `Interchange` API points instead of `Interchange.topology`. + +For example, `Interchange.topology.get_positions()` will never include positions of virtual sites. To get the positions of a system with virtual sites included, use `Interchange.get_positions(include_virtual_sites=True)`. The default behavior of `Interchange.positions` is also to return positions without virtual sites. + ## Quirks of `from_openmm` ### Modified masses are ignored diff --git a/openff/interchange/components/interchange.py b/openff/interchange/components/interchange.py index bc962f82..7ce10a6d 100644 --- a/openff/interchange/components/interchange.py +++ b/openff/interchange/components/interchange.py @@ -288,6 +288,25 @@ def minimize( else: raise NotImplementedError(f"Engine {engine} is not implemented.") + def get_positions(self, include_virtual_sites: bool = False) -> Quantity: + """ + Get the positions associated with this Interchange. + + Parameters + ---------- + include_virtual_sites : bool, default=False + Include virtual sites in the returned positions. + + Returns + ------- + positions : openff.units.Quantity + The positions of the atoms in the system. + + """ + from openff.interchange.interop.common import _to_positions + + return _to_positions(self, include_virtual_sites=include_virtual_sites) + def to_gromacs( self, prefix: str, @@ -616,8 +635,6 @@ def to_openmm_simulation( """ import openmm.app - from openff.interchange.interop.openmm._positions import to_openmm_positions - system = self.to_openmm_system( combine_nonbonded_forces=combine_nonbonded_forces, add_constrained_forces=add_constrained_forces, @@ -641,7 +658,7 @@ def to_openmm_simulation( # include_virtual_sites could possibly be False if self.positions is not None: simulation.context.setPositions( - to_openmm_positions(self, include_virtual_sites=True), + self.get_positions(include_virtual_sites=True).to_openmm(), ) return simulation diff --git a/openff/interchange/interop/common.py b/openff/interchange/interop/common.py index 37910b11..4b4e5bff 100644 --- a/openff/interchange/interop/common.py +++ b/openff/interchange/interop/common.py @@ -1,11 +1,43 @@ """Utilities for interoperability with multiple packages.""" +from openff.toolkit import Quantity + from openff.interchange import Interchange -from openff.interchange.exceptions import UnsupportedExportError +from openff.interchange.exceptions import ( + MissingPositionsError, + MissingVirtualSitesError, + UnsupportedExportError, +) from openff.interchange.models import VirtualSiteKey from openff.interchange.smirnoff import SMIRNOFFVirtualSiteCollection +def _to_positions( + interchange: "Interchange", + include_virtual_sites: bool = False, +) -> Quantity: + """Generate an array of positions of all particles, optionally including virtual sites.""" + if interchange.positions is None: + raise MissingPositionsError( + f"Positions are required, found {interchange.positions=}.", + ) + + if include_virtual_sites: + from openff.interchange.interop._virtual_sites import ( + get_positions_with_virtual_sites, + ) + + try: + return get_positions_with_virtual_sites( + interchange, + use_zeros=False, + ) + except MissingVirtualSitesError: + return interchange.positions + else: + return interchange.positions + + def _check_virtual_site_exclusion_policy(handler: "SMIRNOFFVirtualSiteCollection"): _SUPPORTED_EXCLUSION_POLICIES = ("parents",) diff --git a/openff/interchange/interop/openmm/_positions.py b/openff/interchange/interop/openmm/_positions.py index 6828bfde..143efe1b 100644 --- a/openff/interchange/interop/openmm/_positions.py +++ b/openff/interchange/interop/openmm/_positions.py @@ -4,10 +4,7 @@ from typing import TYPE_CHECKING -from openff.interchange.exceptions import ( - MissingPositionsError, - MissingVirtualSitesError, -) +from openff.interchange.interop.common import _to_positions if TYPE_CHECKING: import openmm.unit @@ -17,25 +14,7 @@ def to_openmm_positions( interchange: "Interchange", - include_virtual_sites: bool = True, + include_virtual_sites: bool = False, ) -> "openmm.unit.Quantity": """Generate an array of positions of all particles, optionally including virtual sites.""" - if interchange.positions is None: - raise MissingPositionsError( - f"Positions are required, found {interchange.positions=}.", - ) - - if include_virtual_sites: - from openff.interchange.interop._virtual_sites import ( - get_positions_with_virtual_sites, - ) - - try: - return get_positions_with_virtual_sites( - interchange, - use_zeros=False, - ).to_openmm() - except MissingVirtualSitesError: - return interchange.positions.to_openmm() - else: - return interchange.positions.to_openmm() + return _to_positions(interchange, include_virtual_sites).to_openmm()