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.

Sign up for GitHub

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

Fix multiindex error on upstream xr #1450

Merged
merged 4 commits into from
Aug 3, 2023
Merged

Fix multiindex error on upstream xr #1450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e35530b
Fix multiindex error on upstream xr
aulemahal Aug 3, 2023
9b80f6f
fix changes
aulemahal Aug 3, 2023
79c603b
Merge branch 'master' into fix-upsxr-percdoy
aulemahal Aug 3, 2023
db78225
Woups remove changes brought over from my stash.
aulemahal Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Internal changes
* Increased the guess of number of quantiles needed in ExtremeValues. (:pull:`1413`).
* Tolerance thresholds for error in ``test_processing::test_adapt_freq`` have been relaxed to allow for more variation in the results. (:issue:`1417`, :pull:`1418`).
* Added 'streamflow' to the list of known variables (:pull:`1431`).
* Fix and adapt ``percentile_doy`` for an error raised by xarray > 2023.7.0 (:issue:`1417`, :pull:`1450`).

v0.44.0 (2023-06-23)
--------------------
Expand Down
9 changes: 8 additions & 1 deletion xclim/core/calendar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,7 +675,14 @@ def percentile_doy(
(rr.time.dt.year.values, rr.time.dt.dayofyear.values),
names=("year", "dayofyear"),
)
rrr = rr.assign_coords(time=ind).unstack("time").stack(stack_dim=("year", "window"))
if hasattr(xr, "Coordinates"):
# xarray > 2023.7.0 will deprecate passing a Pandas MultiIndex directly.
# TODO: Remove this condition when pinning xarray above 2023.7.0
Comment on lines +679 to +680
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can never figure out whether it makes to wrap this in a try/except, perform a version check, make some other conditional check.

This looks good, but we have these kinds of workarounds scattered throughout the code base, and I imagine that some of these are now obsolete. We should perform an inventory of these at some point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The thing with this one is that the other line does work even with the newest master of xarray! But there's discussions to deprecate it.

I thought that with a "TODO:" it would be easier to find ? But yes indeed, we could try to find all version-dependent code paths at some point and prune what's not needed anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like to add a theme to my todo like TODO: [version-specific] .... or TODO: [deprecation] ...
It make it slightly easier to grep and make an inventory.

ind = xr.Coordinates.from_pandas_multiindex(ind, "time")
rr = rr.drop_vars("time").assign_coords(ind)
else:
rr = rr.drop_vars("time").assign_coords(time=ind)
rrr = rr.unstack("time").stack(stack_dim=("year", "window"))

if rrr.chunks is not None and len(rrr.chunks[rrr.get_axis_num("stack_dim")]) > 1:
# Preserve chunk size
Expand Down