Skip to content

Conversation

@dopplershift
Copy link
Member

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

@dopplershift dopplershift added Area: Units Pertains to unit information Type: Enhancement Enhancement to existing functionality labels Dec 21, 2021
@dopplershift dopplershift added this to the 1.2.0 milestone Dec 21, 2021
@dopplershift
Copy link
Member Author

@jthielen What do you think of this?

@dopplershift dopplershift modified the milestones: 1.2.0, 1.3.0 Jan 14, 2022
@dopplershift dopplershift modified the milestones: 1.3.0, May 2022 Mar 31, 2022
@dopplershift dopplershift modified the milestones: May 2022, July 2022 May 16, 2022
@dopplershift dopplershift marked this pull request as ready for review September 15, 2022 19:24
@dopplershift dopplershift requested a review from a team as a code owner September 15, 2022 19:24
@dopplershift dopplershift requested review from dcamron and removed request for a team September 15, 2022 19:24
@jthielen
Copy link
Collaborator

jthielen commented Sep 28, 2022

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 jthielen@6f4c090 (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 #2672.

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.
@dopplershift
Copy link
Member Author

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

@jthielen
Copy link
Collaborator

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
jthielen previously approved these changes Sep 28, 2022
@dopplershift
Copy link
Member Author

dopplershift commented Sep 28, 2022

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?

@jthielen
Copy link
Collaborator

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

@dopplershift
Copy link
Member Author

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

@dopplershift
Copy link
Member Author

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

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.
@dcamron dcamron merged commit 44899a8 into Unidata:main Oct 3, 2022
@dcamron
Copy link
Member

dcamron commented Oct 3, 2022

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

@dopplershift dopplershift deleted the pint-registry branch October 3, 2022 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Units Pertains to unit information Type: Enhancement Enhancement to existing functionality

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Improve setup of pint UnitRegistry

3 participants