Skip to content
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

Remove unit registry, use cf-xarray's or any other. #57

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

aulemahal
Copy link

@aulemahal aulemahal commented Jan 10, 2025

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
  • (If applicable) Documentation has been added / updated (for bug fixes / features).
  • (If applicable) Tests have been added.
  • CHANGELOG.rst has been updated (with summary of main changes).
    • Link to issue (:issue:number) and pull request (:pull:number) has been added.

What kind of change does this PR introduce?

  • Removes the unit registry declaration, instead use the one set as pint's "application registry". In most cases, this will be the one built by cf-xarray, but starting with xclim 0.55, once xclim is imported it will be xclim's.
  • Patch all units functions for the two caveats of cf-xarray's registry:
    • A scalar Quantity.magnitude is a zero-dim array, but we expect a float. I cast them explicitly everywhere we handle this.
    • The cf unit formatter is created but not set as default by cf-xarray. I explicitly use it everywhere we cast to string.
    • Remove uses of pct for "percent" as this is not CF and not supported by cf-xarray.

As we still expect CF outputs, this is not completely agnostic : the unit registry used must come from cf-xarray. But it does allow to use the one from xclim.

Does this PR introduce a breaking change?

Shouldn't.

Other information:

Once Ouranosinc/xclim#2040 is merged a simple import xclim should activate xclim's registry. Then, using the hydro context would look like:

import xsdba
import xarray as xr
from xsdba.testing.utils import gosset
go = gosset()

hist = xr.open_dataset(go.fetch("sdba/CanESM2_1950-2100.nc")).pr.sel(time=slice('1980', '2010'))
ref = xr.open_dataset(go.fetch("sdba/ahccd_1950-2013.nc")).pr.sel(time=slice('1980', '2010'))

# Naively
QDM = xsdba.QuantileDeltaMapping.train(ref=ref, hist=hist)
# Fails with
# DimensionalityError: Cannot convert from 'kilogram / meter ** 2 / second' ([mass] / [length] ** 2 / [time]) to 'millimeter / day' ([length] / [time])

import xclim  # (as of 0.55) this modifies the unit registry _IN PLACE_
with xclim.core.units.units.context('hydro'):
     QDM = xsdba.QuantileDeltaMapping.train(ref=dref.pr.sel(time=slice('1980', '2010')), hist=dsim.pr.sel(time=slice('1980', '2010')))
# Works! Wow!

I will be adding a doc example soon.

Copy link

Welcome, new contributor!

It appears that this is your first Pull Request. To give credit where it's due, we ask that you add your information to the AUTHORS.rst and .zenodo.json:

  • The relevant author information has been added to AUTHORS.rst and .zenodo.json.

Please make sure you've read our contributing guide. We look forward to reviewing your Pull Request shortly ✨

Copy link
Contributor

@coxipi coxipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, a lot cleaner!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possibility of a meta context=="hydro"
2 participants