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

(fix): extension array indexers #9671

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

Conversation

ilan-gold
Copy link
Contributor

Identical to kmuehlbauer#1 - probably not very helpful in terms of changes since https://github.com/kmuehlbauer/xarray/tree/any-time-resolution-2 contains most of it....

kmuehlbauer and others added 30 commits October 18, 2024 07:31
…ore/variable.py to use any-precision datetime/timedelta with autmatic inferring of resolution
…t resolution, fix code and tests to allow this
… more carefully, for now using pd.Series to covert `OMm` type datetimes/timedeltas (will result in ns precision)
…rray` series creating an extension array when `.array` is accessed
@benbovy
Copy link
Member

benbovy commented Dec 10, 2024

Unfortunately I'm not able to review all changes related to datetime types, but I just wanted to note that the changes in core/indexes.py and core/indexing.py will likely solve xarray-contrib/xvec#87.

Any blocker apart from the CI failures and merge conflicts?

Doctest errors should be trivial to fix (although looking at the errors this makes me think that pandas Extension types might cause some rendering issues of Xarray variable inline reprs, depending on how long the reprs of the extension types can be).

@kmuehlbauer
Copy link
Contributor

@benbovy Those changes have been added on top of #9618, which has diverged from the contents here. I'm not sure what the best procedure is, but maybe disentangling this PR from #9618 would work?

@ilan-gold
Copy link
Contributor Author

@benbovy Those changes have been added on top of #9618, which has diverged from the contents here. I'm not sure what the best procedure is, but maybe disentangling this PR from #9618 would work?

I'm happy to update the PR here

@ilan-gold
Copy link
Contributor Author

maybe disentangling this PR

The reason I created my PR on top of yours was that I spent many hours trying to do what you did, but much worse. The issue is that datetime + extension array permissibility are quite interconnected.

@ilan-gold
Copy link
Contributor Author

@kmuehlbauer If you fix the merge conflicts with main I can clean up the diff in the PR to your repo so people can review there. And then we'll have CI here

@kmuehlbauer
Copy link
Contributor

If you fix the merge conflicts with main I can clean up the diff in the PR to your repo so people can review there. And then we'll have CI here

Thanks @ilan-gold for the details. I've fixed the conflicts over in #9618. Would be great to get this over the line soon :-)

@ilan-gold
Copy link
Contributor Author

@benbovy Cleaner diff over in https://github.com/kmuehlbauer/xarray/pull/1/files

@benbovy
Copy link
Member

benbovy commented Dec 11, 2024

Thanks @ilan-gold. After a brief look it looks like kmuehlbauer#1 could be opened here as a separate PR targeting pydata/xarray/main, or maybe I'm wrong?

@ilan-gold
Copy link
Contributor Author

@benbovy Is that not what this PR is?

@benbovy
Copy link
Member

benbovy commented Dec 11, 2024

I was actually referring to #9671 (comment), but waiting for #9618 to be merged may be fine too I guess. The cleaner diff over in https://github.com/kmuehlbauer/xarray/pull/1/files makes the review easier although CI here still mixes those changes with #9618.

@ilan-gold
Copy link
Contributor Author

CI here still mixes those changes with #9618.

Yeah, it's unfortunate but so many datetime indexers are passed through now and left untouched as a result of the changes here that I either had to restrict the datetime types one-by-one where they pop up or re implement the entirety of #9618 anyway to loosen the restriction globally.

@ilan-gold
Copy link
Contributor Author

Everything working except small mypy stuff and a weird test or two that seem dependent on python version: 8a3e834

I will be on vacation for two-ish weeks starting tomorrow but this PR is probably reviewable, at least in hte other repo and I am happy to work on it as soon as I'm back.

@kmuehlbauer
Copy link
Contributor

Everything working except small mypy stuff and a weird test or two that seem dependent on python version: 8a3e834

#9873

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

Successfully merging this pull request may close these issues.

4 participants