-
Notifications
You must be signed in to change notification settings - Fork 443
Use Pint Application Registry #2272
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
Conversation
|
@jthielen What do you think of this? |
9b1f475 to
5d51227
Compare
|
Overall, this looks good to me! After testing out various permutations of MetPy (before and after this change), pint-xarray, and cf-xarray, this indeed looks like the best approach we can practically take for compatibility. One corner case I discovered: if you import cf-xarray's registry (which defines a new registry and sets it with import xarray as xr
import pint_xarray
from metpy.units import units
data_pre = xr.DataArray([3, 5, 7] * units('m/s'))
print(data_pre.pint.units, data_pre.metpy.units)
import cf_xarray.units
data_post = xr.DataArray([3, 5, 7] * units('m/s'))
print(data_pre.pint.units, data_pre.metpy.units)
print(data_post.pint.units, data_post.metpy.units)This should be fixable by jthielen@6f4c090 (comparing against pint's top-level |
UnitStrippedWarning is in our full range of supported versions of Pint.
Instead of blindly creating our own registry and using it, instead use the application registry and modify it to our needs. I'm not wild about modifying global objects shared by different libraries, but I think it is better than us insisting on our own registry--which has no hope of playing well with another library.
5d51227 to
3243443
Compare
|
@jthielen Does |
The top-level |
|
Up to this point, we've been able to limit "direct" use of def isquantity(*args):
return all(isinstance(a, pint.Quantity) for a in args)that lives in |
Sure! I hadn't considered there to be downsides to directly importing pint elsewhere in the codebase. Though, I'd think having that be |
|
I don't think having it direct is horrible, but I think it's marginally better organization to keep the "implementation detail" of using Pint (types in our docs aside) confined as much as possible. It leaves us in better shape if we need to swap out Pint.
I sneakily edited my comment to do that very thing now right before I saw your reply. 😉 |
|
And to be clear, I'll do the refactor in lieu of a88d7dd. |
a88d7dd to
459fddc
Compare
Instead of checking against Quantity from our registry, just use pint.Quantity, which should effectively be a base class for any instance. This goes some way to helping with messy interoperability with other pint-using libraries, like cf_xarray.
459fddc to
43c9c8c
Compare
|
Thanks for the testing and helping get this in @jthielen! |
Description Of Changes
Instead of blindly creating our own registry and using it, instead use the application registry and modify it to our needs. I'm not wild about modifying global objects shared by different libraries, but I think it is better than us insisting on our own registry--which has no hope of playing well with another library.
Also bumping our minimum Pint version to 0.11 to let us clean up some warning handling.
Checklist
- [ ] Tests added- [ ] Fully documented