-
Notifications
You must be signed in to change notification settings - Fork 59
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
Fix multiindex error on upstream xr #1450
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
# xarray > 2023.7.0 will deprecate passing a Pandas MultiIndex directly. | ||
# TODO: Remove this condition when pinning xarray above 2023.7.0 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Pull Request Checklist:
number
) and pull request (:pull:number
) has been addedWhat kind of change does this PR introduce?
Overriding a coordinate with a pandas MultiIndex was not working properly before, but this didn't result in any errors up to a recent change in xarray's master. When calling
assign_coords
with a multiindex on a coordinate that already existed, the sub-indexes would not appear in the DataArray repr. In the most recent xarray master, that now triggers an error when trying to unstack the multiindex, but only if dask is used. Before, the unstacking would proceed as normal and the sub-indexes would magically appear on the result. Dropping the previous coordinate solves this problem.The next xarray will implement a new public
xr.Coordinates
class that should be used to assign coords, instead of a rawpd.MultiIndex
. Nonetheless, overriding is still not possible and dropping the previous coordinate is still needed.Does this PR introduce a breaking change?
No.
Other information:
See pydata/xarray#8039.