issue_comments

I'm going to merge this to see if it fixes our doc build...

It's also worth noting that this note appears to be causing our doc builds to fail: https://readthedocs.org/projects/xray/builds/10604464/

These changes look good to me. It's definitely not ideal to be overriding all these details on CFTimeIndex -- this why xarray needs its own indexing API -- but for now it seems like about the best we can do.

Hello @spencerkclark! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2020-03-09 07:37:46 UTC

+1 for marking as xfail. There’s no point in keeping the master branch failing if we know what the issue is.

On Thu, Feb 27, 2020 at 7:22 PM Maximilian Roos notifications@github.com wrote:

Re the specific issue, that's a tough one. It could be worth floating on pandas and seeing if they have thoughts...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/3764?email_source=notifications&email_token=AAJJFVXTH5QBZHOH47UZIQDRFB7NXA5CNFSM4KTAWZ7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENG2V6Y#issuecomment-592292603, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJJFVQKSGKMKA35V4JDV4DRFB7NXANCNFSM4KTAWZ7A .

Re the specific issue, that's a tough one. It could be worth floating on pandas and seeing if they have thoughts...

Re master failing, what do we think about xfail-ing those tests on master and then adding a PR which undoes the xfail and attempts fixes the issue?

I'm cognizant that every PR now fails, which less friendly to contributors. The project home page also states .

ping @shoyer for input.

I added some updates to this PR this morning that in principle would solve the indexing with method="nearest" from within xarray. Unfortunately though, due to the issue I described in https://github.com/pydata/xarray/pull/3764#issuecomment-586597512, I was not able to come up with a solution that did not require overriding the pandas implementation of _get_nearest_indexer. If this seems unacceptable, maybe we can think harder about how we might address this upstream instead (e.g. I think special-casing the change made in https://github.com/pandas-dev/pandas/pull/31511 to just DatetimeIndexes, and preserving the old behavior for everything else, could be sufficient).

In addition, for the time being I xfailed test_indexing_in_series_getitem, as I think there is agreement that that would be best addressed upstream, https://github.com/pydata/xarray/issues/3751#issuecomment-587572443.

Finally in the process of doing this, I cleaned up the implementation of CFTimeIndex.__sub__, and added some more test coverage; hopefully now it's a little clearer what cases it's supposed to work for and what cases it is not.

This is a little bit trickier than I originally anticipated. For indexing with dates very distant from the dates in the index, I'm still running into an issue at this step in pandas.Index._get_nearest_indexer: left_distances = np.abs(self[left_indexer] - target) Consider the following the example: ``` In [1]: import numpy as np; import pandas as pd; import xarray as xr

In [2]: import cftime

In [3]: times = xr.cftime_range('0001', periods=4)

In [4]: da = xr.DataArray(range(4), coords=[('time', times)])

In [5]: da.sel(time=cftime.DatetimeGregorian(2000, 1, 1), method='nearest') ```

In this example self[left_indexer] is CFTimeIndex([0001-01-04 00:00:00], dtype='object', name='time'), while target is Index([2000-01-01 00:00:00], dtype='object'). The distance between these dates is greater than 292 years, so it cannot be represented in a TimedeltaIndex.

One could argue that we could fall back on using a generic Index of dtype object to store the datetime.timedelta object produced in this case: ``` In [6]: left_index = xr.CFTimeIndex([cftime.DatetimeGregorian(1, 1, 4)])

In [7]: target = pd.Index([cftime.DatetimeGregorian(2000, 1, 1)])

In [8]: difference = left_index - target

In [9]: difference Out[9]: Index([-730118 days, 0:00:00], dtype='object') `` A problem occurs though when we try to take the absolute value of this index. Pandas (I think reasonably so) tries to detect the datatype of the result and construct a new index. In doing so it tries to create aTimedeltaIndex, but cannot, because thetimedelta` inside is too large:

So I'm a little bit back to the drawing board regarding a solution for this. Part of me is tempted to write an overriding version of _get_nearest_indexer for CFTimeIndex that basically restores the old behavior; I'm not sure how dangerous that would be to rely on though.