New `to_openmm_positions` virtual sites behaviour breaks consistency with other `to_openmm_X` methods

[IAlibay]

Follow-up from #1054 and #1074

From @mattwthompson :

I've changed the default behavior of the OpenMM positions getter to exclude virtual sites by default - arguably what it should have always done

The new behaviour in interchange is now that:

1. to_openmm_system and to_openmm_topology output virtual sites by default
2. No OpenMM position methods will return by default virtual sites

I can see the point of having consistency across position getters, but this possibly worsens the user/developer experience. Personally I would expect that all to_openmm_X methods behave in the same way / work together.

My alternate proposal would be:

a) we know/live with the idea that the interchange.positions.to_openmm method is just broken for now (stick a big warning box in the docs or wrap a warning around the method), b) expect to_openmm_positions to be the thing we need to use, c) expect to_openmm_X to all behave in the same way

One additional (and as a developer a big) reason for this opinion is that it avoids needing to deal with an API break that is likely to get reverted in the future. Yes the answer here is just "well specify the kwarg value in your code", and I do, but it's nice when you forget to do this and stuff doesn't break between releases.

[mattwthompson]

I'm pretty sure @j-wags and I arrived at the same idea earlier today while hashing through some of this. The patch I'm wanting to apply basically keeps #1074 as-is but flips the default behavior to include virtual sites where possible. (The main downside is the inconsistency with Interchange.topology.get_positions().to_openmm(), but that's a mess to fix and is a documented limitation.)

Is this roughly what you had in mind?

diff --git a/openff/interchange/components/interchange.py b/openff/interchange/components/interchange.py
index 7ce10a6d..07373305 100644
--- a/openff/interchange/components/interchange.py
+++ b/openff/interchange/components/interchange.py
@@ -288,13 +288,13 @@ class Interchange(_BaseModel):
         else:
             raise NotImplementedError(f"Engine {engine} is not implemented.")

-    def get_positions(self, include_virtual_sites: bool = False) -> Quantity:
+    def get_positions(self, include_virtual_sites: bool = True) -> Quantity:
         """
         Get the positions associated with this Interchange.

         Parameters
         ----------
-        include_virtual_sites : bool, default=False
+        include_virtual_sites : bool, default=True
             Include virtual sites in the returned positions.

         Returns
diff --git a/openff/interchange/interop/common.py b/openff/interchange/interop/common.py
index 4b4e5bff..14024507 100644
--- a/openff/interchange/interop/common.py
+++ b/openff/interchange/interop/common.py
@@ -14,9 +14,9 @@ from openff.interchange.smirnoff import SMIRNOFFVirtualSiteCollection

 def _to_positions(
     interchange: "Interchange",
-    include_virtual_sites: bool = False,
+    include_virtual_sites: bool = True,
 ) -> Quantity:
-    """Generate an array of positions of all particles, optionally including virtual sites."""
+    """Generate an array of positions of all particles, optionally excluding virtual sites."""
     if interchange.positions is None:
         raise MissingPositionsError(
             f"Positions are required, found {interchange.positions=}.",
diff --git a/openff/interchange/interop/openmm/_positions.py b/openff/interchange/interop/openmm/_positions.py
index 143efe1b..2a4ce2cb 100644
--- a/openff/interchange/interop/openmm/_positions.py
+++ b/openff/interchange/interop/openmm/_positions.py
@@ -14,7 +14,7 @@ if TYPE_CHECKING:

 def to_openmm_positions(
     interchange: "Interchange",
-    include_virtual_sites: bool = False,
+    include_virtual_sites: bool = True,
 ) -> "openmm.unit.Quantity":
-    """Generate an array of positions of all particles, optionally including virtual sites."""
+    """Generate an array of positions of all particles, optionally excluding virtual sites."""
     return _to_positions(interchange, include_virtual_sites).to_openmm()

[IAlibay]

Is this roughly what you had in mind?

Yes!

[mattwthompson]

Oh, there's a downside of Interchange.to_openmm_positions() and the lightly-wrapped Interchange.get_positions() including virtual sites but Interchange.positions not. I'll have to think about this a little more - maybe the simple Interchange.get_positions() was a bad idea.