Simplify unit handling in some setters #1570
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattwthompson
commented
Apr 6, 2023
Comment on lines
411
to
420
| def test_set_partial_charges_openmm_quantity(self, water): | ||
| import openmm.unit | ||
|
|
||
| with pytest.raises(ValueError, match="Cannot set.*openmm.unit"): | ||
| water.atoms[2].partial_charge = 0.0 * openmm.unit.elementary_charge | ||
|
|
There was a problem hiding this comment.
This could be added back (I forget why I removed it) but the error message will probably be difference since this is no longer being handled internally.
mattwthompson
force-pushed
the
simpler-units
branch
2 times, most recently
from
6566b08
to
4e58a81
Compare
Co-authored-by: Jeff Wagner <[email protected]>
mattwthompson
force-pushed
the
simpler-units
branch
from
4e58a81
to
a0050fe
Compare
j-wags
approved these changes
Oct 18, 2023
Comment on lines
-165
to
-166
| return str(input_quantity) | ||
|
|
There was a problem hiding this comment.
😱 How... long has this been around...?
Comment on lines
-414
to
-420
| @requires_pkg("openmm") | ||
| def test_set_partial_charges_openmm_quantity(self, water): | ||
| import openmm.unit | ||
|
|
||
| with pytest.raises(ValueError, match="Cannot set.*openmm.unit"): | ||
| water.atoms[2].partial_charge = 0.0 * openmm.unit.elementary_charge | ||
|
|
There was a problem hiding this comment.
IIUC, this is no longer an error because the openmm Quantities are now valid inputs.
|
Thanks for the review - I'll have to look back and see why I wanted these changes in the first place and if they're still needed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
At various points in time we've needed to bake in logic to gracefully handle similar but heterogenous inputs. Each implementation has diverged over time; this change consolidates them and funnels most of the logic to a single choke point.