Merge pull request #1074 from openforcefield/high-level-positions-getter

Add high-level positions getter, exclude virtual sites by default

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

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

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",)
 

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()