MetPy icon indicating copy to clipboard operation
MetPy copied to clipboard

Use Pint Application Registry

Open dopplershift opened this issue 2 years ago • 1 comments

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~

dopplershift avatar Dec 21 '21 03:12 dopplershift

@jthielen What do you think of this?

dopplershift avatar Dec 21 '21 03:12 dopplershift

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 Quantitys 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 avatar Sep 28 '22 01:09 jthielen

@jthielen Does pint.Quantity always refer to the current set registry?

dopplershift avatar Sep 28 '22 01:09 dopplershift

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).

jthielen avatar Sep 28 '22 02:09 jthielen

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?

dopplershift avatar Sep 28 '22 15:09 dopplershift

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 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)?

jthielen avatar Sep 28 '22 15:09 jthielen

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 than any 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:

dopplershift avatar Sep 28 '22 16:09 dopplershift

And to be clear, I'll do the refactor in lieu of a88d7dd.

dopplershift avatar Sep 28 '22 16:09 dopplershift

Thanks for the testing and helping get this in @jthielen!

dcamron avatar Oct 03 '22 20:10 dcamron