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

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Jan 30, 2023

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.

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Merging #1528 (7c91984) into main (c49e483) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@mattwthompson mattwthompson marked this pull request as ready for review January 30, 2023 22:24
@mattwthompson mattwthompson requested a review from j-wags January 30, 2023 22:24
Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the super clear test.

@mattwthompson mattwthompson force-pushed the fix-openmm-box-vectors branch from 767ab28 to c460fc2 Compare February 2, 2023 01:05
@mattwthompson mattwthompson merged commit 54706a1 into main Feb 2, 2023
@mattwthompson mattwthompson deleted the fix-openmm-box-vectors branch February 2, 2023 03:15
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants