bump openff-units and pint pins #1374
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1374 +/- ##
==========================================
- Coverage 94.63% 92.47% -2.17%
==========================================
Files 143 143
Lines 10962 10962
==========================================
- Hits 10374 10137 -237
- Misses 588 825 +237
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I believe by doing this bump, we will have to fully migrate to pydantic v2+ (iirc interchange v0.4 and above is pydantic 2 only and openff-units 0.3 is does not work with pre-0.4 versions of interchange). |
|
Regarding the mypy failures, this is what we're seeing here: OpenFreeEnergy/gufe#533 We'll need to import Quantity directly from openff.units for typing. |
|
For the EitherQuantity failure, the best answer will be to just directly call "from_openmm" instead. kT in ThermodynamicState should be in OpenMM units. |
|
the last nagging mypy errors are: I'm going to ignore these to get this PR merged, but im curious if this is expected or fixable @mattwthompson (I see from openff-units that this inherits directly from pint and therefore might be out of your control). |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
I'm going to put a block as "I need to review this", since it llikely means other ongoing work needs fixing.
mattwthompson
left a comment
mattwthompson
left a comment
There was a problem hiding this comment.
unit.Quantity and Quanitity should be completely substitute-able with each other. Is that not the case? / I wonder if these changes are motivated by runtime changes or attempts to make Mypy happy?
Of course, it's possible that this is wrong and/or out of date. It might be worth revisiting, though I won't be able to do that until mid-July at the earliest.
The addition of several # type: ignores is surely unfortunate but the code should work fine otherwise?
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Couple of things, otherwise lgtm.
|
|
||
| sim_time = round(sim_length.to('attosecond').m) | ||
| ts = round(timestep.to('attosecond').m) | ||
| sim_time = round(sim_length.to('attosecond').m) # type: ignore |
There was a problem hiding this comment.
Oddly enough, mypy does define __round__, so we should be able to call round on the attosecond Quantity and then call its magnitude, but something is going wrong here.
Looks like there's some stalled PRs on mypy to deal partially with this.
| 'forward_dDGs': unit.Quantity.from_list(forward_dDGs), | ||
| 'reverse_DGs': unit.Quantity.from_list(reverse_DGs), | ||
| 'reverse_dDGs': unit.Quantity.from_list(reverse_dDGs) | ||
| 'forward_DGs': Quantity.from_list(forward_DGs), # type: ignore |
There was a problem hiding this comment.
These are all because the pyi from openff-units doesn't define the method. Manually adding it fixes the problem. Raising the issue here: openforcefield/openff-units#127
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
just one spare space that needs removing
|
No API break detected ✅ |
I think we were pinned to
openff-units==0.2.0andpint<0.22because we were waiting on this: OpenFreeEnergy/gufe#533.Trying an unpin now to see where we're at.
Checklist
newsentryDevelopers certificate of origin