Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow box vectors to be set with OpenMM Quantity objects #1528

Merged
merged 2 commits into from
Feb 2, 2023
Merged

Allow box vectors to be set with OpenMM Quantity objects #1528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c460fc2
Automatically convert `openmm.unit.Quantity` box vectors
mattwthompson Jan 30, 2023
7c91984
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 2, 2023
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Releases follow the `major.minor.micro` scheme recommended by [PEP440](https://w
### New features
- [PR #1502](https://github.com/openforcefield/openff-toolkit/pull/1502): Adds Gasteiger charge computation using the RDKit backend.
- [PR #1498](https://github.com/openforcefield/openff-toolkit/pull/1498): `Molecule.remap()` now supports partial mappings with the `partial` argument.
- [PR #1528](https://github.com/openforcefield/openff-toolkit/pull/1528): `Topology.box_vectors` are can now be set with `openmm.unit.Quantity`s, which are internally converted.

### Behavior changes
- [PR #1498](https://github.com/openforcefield/openff-toolkit/pull/1498): New, more complete, and more descriptive errors for `Molecule.remap()`.
Expand Down
10 changes: 10 additions & 0 deletions openff/toolkit/tests/test_topology.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,16 @@ def test_box_vectors(self):
topology.box_vectors = good_vectors
assert (topology.box_vectors == good_vectors * np.eye(3)).all()

def test_issue_1527(self):
"""Test the error handling of setting box vectors with an OpenMM quantity."""
import numpy
import openmm.unit

topology = Topology()
topology.box_vectors = numpy.ones(3) * openmm.unit.nanometer

assert isinstance(topology.box_vectors, unit.Quantity)

def test_is_periodic(self):
"""Test the getter and setter for is_periodic"""
vacuum_top = Topology()
Expand Down
9 changes: 8 additions & 1 deletion openff/toolkit/topology/topology.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,14 @@ def box_vectors(self, box_vectors):
self._box_vectors = None
return
if not hasattr(box_vectors, "units"):
raise InvalidBoxVectorsError("Given unitless box vectors")
if hasattr(box_vectors, "unit"):
# this is probably an openmm.unit.Quantity; we should gracefully import OpenMM but
# the chances of this being an object with the two previous conditions met is low
box_vectors = ensure_quantity(box_vectors, "openff")
Copy link
Member Author

Choose a reason for hiding this comment

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

This case could optionally be an error, but I think it's better to just convert it. Setters don't have guarantees around not modifying inputs and the exception would just instruct users to do this.


else:
raise InvalidBoxVectorsError("Given unitless box vectors")

# Unit.compatible_units() returns False with itself, for some reason
if (box_vectors.units != unit.nm) and (
box_vectors.units not in unit.nm.compatible_units()
Expand Down