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

Clean and export the units registry #2040

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

Clean and export the units registry #2040

wants to merge 2 commits into from

Conversation

aulemahal
Copy link
Collaborator

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • Tests for the changes have been added (for bug fixes / features)
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • 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?

  • Remove support for non-CF aliases : "cms" (m3 s-1), "mmday" (mm d-1) and "pct" (%). These are not supported by udunits2 and thus I don't see why we should support them ? @huard, @RondeauG what do you think ? I'm not strongly opinionated on this.

  • Set the unit registry as pint's "application registry" to make it usable by non-xclim aware applications. To be obtained with ureg = pint.get_application_registry().

  • Units of quantities printed in indicator attributes are now formatted using the CF syntax (ex: "mm d-1" instead of "mm/d"). Not sure how I missed that earlier...

  • Clean up in the code of the registry declaration and addition of comments to explain what is happening.

  • A small change in dataflags that was actually unneeded at the end, but it felt cleaner to me, so I kept it.

Does this PR introduce a breaking change?

Some units are not supported anymore (cms, pct and mmday).
Some attributes will be different as the units are printed differently (for fractions mostly).

Other information:

The registry export allows an application/module that is units-agnostic in itself to be used with xclim. For those applications / usages, the import order might be significant though : xclim has to be the last set_application_registry done. Specifically in the climate case, as xclim.core.units imports cf_xarray.units (which also sets the app registry), a subsequent import cf_xarray.units is not problematic as python will not actually execute it.

@coxipi Incoming PR to xsdba to use exactly that.

@aulemahal aulemahal requested a review from Zeitsperre January 10, 2025 22:01
@github-actions github-actions bot added the indicators Climate indices and indicators label Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
indicators Climate indices and indicators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant