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

Phase out convert_calendar and some units processing #1010

Open
1 of 6 tasks
aulemahal opened this issue Feb 7, 2022 · 1 comment
Open
1 of 6 tasks

Phase out convert_calendar and some units processing #1010

aulemahal opened this issue Feb 7, 2022 · 1 comment
Assignees
Labels
API Interfacing and User Concerns
Milestone

Comments

@aulemahal
Copy link
Collaborator

aulemahal commented Feb 7, 2022

Since xarray 0.20.0, some of xclim's calendar functionality has been implemented in xarray directly. Similarly, cf_xarray 0.7.0 now replicates (with minor differences and improvements) the units string processing machinery of xclim. Which means we could phase them out of xclim using these upstream methods instead.

Functions concerned:

  • xc.core.calendar.convert_calendar -> xr.Dataset.convert_calendar, xr.DataArray.convert_calendar, xr.coding.calendar_ops.convert_calendar
    The main difference is that numpy's calendar dtype is the default for "standard" and cftime's one is use when use_cftime=True is requested.

  • xc.core.calendar.interp_calendar -> xr.Dataset.interp_calendar, xr.DataArray.interp_calendar, xr.coding.calendar_ops.interp_calendar

  • xc.core.calendar.date_range -> xr.date_range

  • xc.core.calendar.date_range_like -> xr.date_range_like

  • xc.core.units.units (the pint registry) -> cf_xarray.units.units
    They don't implement our special "hydro" context, and I don't think they ever will.

  • xc.core.units.str2pint -> cf_xarray.units.units.Unit
    Some (most? all?) of the string parsing we do by hand as moved to pint's preprocessor, through cf_xarray's registry.

    UPDATE: We still implement the possibility of passing a magnitude-less unit string OR a full quantity. Also, our units2pint supports for input types and has as some typo catching.

  • xc.core.units.pint2cfunits -> f"{u:~cf}"
    Again, some of things we did by hand were moved to pint's formatters.
    UPDATE: Unable to use custom formatting for dimensionless units hgrecco/pint#1486 still prevents the conversion of dimensionless units to "" using cf-xarray's formatter.

So what is our plan for the deprecation?

I suggest we first simply make use of the upstream methods within our own API and remove them once and for all in xclim 1.0. May be adding a warning now, or at least on the last 0.x version. And we document this in the notebooks and docstrings.

@aulemahal aulemahal added the API Interfacing and User Concerns label Feb 7, 2022
@aulemahal aulemahal added this to the v1.0 milestone Feb 7, 2022
@huard huard modified the milestones: v1.0, v0.38 May 9, 2022
@Zeitsperre Zeitsperre modified the milestones: v0.38, v0.39 Sep 6, 2022
@tlogan2000 tlogan2000 modified the milestones: v0.39, v0.40 Oct 11, 2022
@aulemahal aulemahal mentioned this issue Oct 25, 2022
6 tasks
Zeitsperre added a commit that referenced this issue Oct 25, 2022
### What kind of change does this PR introduce?

* Changes how the `UnitRegistry` is declared, by copying the one from
`cf-xarray`.
* I also copied the declaration of the % unit from `cf-xarray`
* This removed the need to import things that were refactored in
`pint`0.20, so it fixes this.
* It also allowed to remove the string processing stuff from
`units2pint`, this is now directly in the registry's `preprocessors`.
* I removed the `force_ndarray_like=True` option that `cf-xarray` uses
because it made our tests fail. I haven't looked further, this is to be
done in #1010.

### Does this PR introduce a breaking change?
It shouldn't!
@tlogan2000 tlogan2000 modified the milestones: v0.40, v0.41 Nov 8, 2022
@Zeitsperre Zeitsperre modified the milestones: v0.41, Summer 2023 Feb 28, 2023
Zeitsperre added a commit that referenced this issue Mar 23, 2023
### What kind of change does this PR introduce?

* Fixes a bug in the logic of two indices (`snowfall_approximation` with
`method="brown"`, `effective_growing_degree_days`) that was emitting
usage warnings from valid calls.
* prepends `v` to entries in CHANGES.rst to properly generate hyperlink
entries.
* Warnings from set ``_version_deprecated`` within Indicators now emit
`FutureWarning` instead of `DeprecationWarning` for greater visibility.

### Does this PR introduce a breaking change?

No.

### Other information:

See also: #1010
aulemahal added a commit that referenced this issue Jan 11, 2024
<!--Please ensure the PR fulfills the following requirements! -->
<!-- If this is your first PR, make sure to add your details to the
AUTHORS.rst! -->
### Pull Request Checklist:
- [x] This PR addresses an already opened issue (for bug fixes /
features)
    - This PR fulfills some goals of #1010.
- [x] Tests for the changes have been added (for bug fixes / features)
- [x] (If applicable) Documentation has been added / updated (for bug
fixes / features)
- [x] CHANGES.rst has been updated (with summary of main changes)
- [x] Link to issue (:issue:`number`) and pull request (:pull:`number`)
has been added

### What kind of change does this PR introduce?

* The units registry is copied from `cf_xarray` and extended with
xclim's specific shortcuts (`precipitation`, `discharge` and the `hydro`
context).
* The units formatting functions are simplified by using the work
alreadty done in `cf_xarray`. We still can't get rid of `pint2cfunits`
because of pint issue that makes it impossible to format dimensionless
units are "" using its formatter tools.

### Does this PR introduce a breaking change?
Yes. As seen in the tests changes, cf_xarray's cf formatter never adds
the "^" sign for exponentiation. This does not affect the accepted
units, only the output `units` attribute.
@Zeitsperre
Copy link
Collaborator

Zeitsperre commented Mar 12, 2024

This is waiting on upstream changes in xarray.

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

No branches or pull requests

5 participants