-
Notifications
You must be signed in to change notification settings - Fork 22
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
New to_openmm_positions
virtual sites behaviour breaks consistency with other to_openmm_X
methods
#1077
Comments
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 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() |
Yes! |
Oh, there's a downside of |
Follow-up from #1054 and #1074
From @mattwthompson :
The new behaviour in interchange is now that:
to_openmm_system
andto_openmm_topology
output virtual sites by defaultI 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 wayOne 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.
The text was updated successfully, but these errors were encountered: