MetPy
MetPy copied to clipboard
Use Pint Application Registry
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
- [x] Closes #1350 ~- [ ] Tests added~ ~- [ ] Fully documented~
@jthielen What do you think of this?
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 pint.set_application_registry
), previously defined Quantity
s start failing some of MetPy's internal isinstance(thing, units.Quantity)
checks. For example, while contrived, this demonstrates the issue
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)
meter / second meter / second
meter / second dimensionless
meter / second meter / second
This should be fixable by https://github.com/jthielen/MetPy/commit/6f4c090fe32faa5e217605ec0b8e1d9be4cb165f (comparing against pint's top-level Quantity
rather than our imported registry's Quantity
). While we definitely can't expect to fully support mixed registries (given that Pint itself usually doesn't), by changing these checks, we can fix errors like 'u
requires "[speed]" but given "dimensionless"' that would show up in situations like this and https://github.com/Unidata/MetPy/issues/2672.
@jthielen Does pint.Quantity
always refer to the current set registry?
Does
pint.Quantity
always refer to the current set registry?
The top-level pint.Quantity
as a class is in-effect a superclass of any registry's Quantity
(even though using it as a constructor will return a Quantity belonging to the application registry)...the actual details of inheritance are really convoluted (with stuff like this and all the facets and such).
Up to this point, we've been able to limit "direct" use of pint
to metpy.units
, so I don't love that this is bleeding more uses of import pint
across the codebase. Would it make sense to refactor the isinstance(foo, pint.Quantity)
calls to something like:
def isquantity(*args):
return all(isinstance(a, pint.Quantity) for a in args)
that lives in metpy.units
?
Up to this point, we've been able to limit "direct" use of
pint
tometpy.units
, so I don't love that this is bleeding more uses ofimport pint
across the codebase. Would it make sense to refactor theisinstance(foo, pint.Quantity)
calls to something like:def isquantity(*args): return any(isinstance(a, pint.Quantity) for a in args)
that lives in
metpy.units
?
Sure! I hadn't considered there to be downsides to directly importing pint elsewhere in the codebase. Though, I'd think having that be all
rather than any
would be better (or just use a single arg)?
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.
Though, I'd think having that be
all
rather thanany
would be better (or just use a single arg)?
I sneakily edited my comment to do that very thing now right before I saw your reply. :wink:
And to be clear, I'll do the refactor in lieu of a88d7dd.
Thanks for the testing and helping get this in @jthielen!