OK, I'm happy with this. Time to merge I guess?

A couple other things to think about from a usability perspective: - What happens when you try to resample along CFTimeIndex? - What happens when you try to plot a DataArray with a CFTimeIndex?

These should at least raise informative errors (use NotImplementedError).

See also https://github.com/pydata/xarray/pull/2098, which should fix failing builds on master

We did this already in #2054, which is why I suspect this is an upstream packaging issue. Right now on conda-forge the win-32 version of cftime is still v1.0.0a3, while the other platforms' versions have been updated to v1.0.0b1, which has the updates we need for cftime.num2date.

OK. In that case, we should probably add a temporary "pip" clause to the requirements file for windows, to install cftime from pypi instead for now. I don't want to merge this when it will break CI.

With regards to CI failures, I think it would make sense to add an explicit dependency on cftime rather than relying on an implicit dependency through the netCDF4 package. You can do this by editing the requirements.yml files in the ci/ directory. This would presumably resolve the issue with inadvertently using an old version of cftime.

It looks like one of the appveyor builds is failing due to something related to cftime missing -- it would be good to fix that

I think the code is in pretty good shape here.

My main concern is about the stability of the netcdftime package, which it looks like might need to be renamed? https://github.com/Unidata/netcdftime/issues/36

As for resampling, this would indeed require custom logic for netcdf datetimes. But I think it would be relatively doable. The key thing would dividing an array of datetimes into frequency groups. Then we could reuse xarray's existing logic for resampling, e.g., https://github.com/pydata/xarray/blob/9b76f219ec314dcb0c9a310c097a34f5c751fdd6/xarray/core/groupby.py#L234-L235

For example, if using freq='1AS' (annual frequency, start), we would need a function that maps netcdftime.datetime objects onto a netcdftime.datetime object corresponding to the start of a year, e.g., datetime(2000, 1, 1) -> datetime(2000, 1, 1) datetime(2000, 1, 2) -> datetime(2000, 1, 1) datetime(2000, 1, 3) -> datetime(2000, 1, 1) ... datetime(2000, 12, 31) -> datetime(2000, 1, 1) datetime(2001, 1, 1) -> datetime(2001, 1, 1) ... datetime(2001, 12, 31) -> datetime(2001, 1, 1)

Pandas does this logic with offset classes. These are somewhat complex because pandas handles complex business day logic. For netcdftime, we could potentially start from scratch and only handle the important cases for climate science (e.g., round to start for year, quarter, month, day, hour, second).

I think netcdftime should have a version of num2date that always returns netcdftime.datetime objects. It's really error prone to functions that turn different types dependent on input values.

On Tue, Mar 20, 2018 at 11:39 AM Spencer Clark notifications@github.com wrote:

It occurred to me that netcdftime.num2date can return datetime.datetime objects in certain circumstances (not just objects that subclass netcdftime.datetime). 5e1c4a8 https://github.com/pydata/xarray/commit/5e1c4a891c22f3c6b54d31ece8c29306483b36a0 provides fixes to allow things to work with datetime.datetime objects as well as more test coverage.

One thing that makes the logic a bit messier here is that determining the calendar type for encoding datetime.datetime objects for faithful roundtripping https://github.com/spencerkclark/xarray/blob/257f08607c3b0cb975a5114948d2f95f941543db/xarray/coding/times.py#L248-L268 is a bit complicated. As far as I can tell, there are two scenarios, each with their own branches of logic:

1. Arrays where the minimum date is before the start of the Gregorian calendar (1582-10-15)
   • If the dates have the type datetime.datetime we encode them using the 'gregorian' calendar.
   • If the dates have the type netcdftime.DatetimeGregorian we encode them using the 'standard' calendar type.
2. Arrays where the minimum date is on or after the start of the Gregorian calendar (1582-10-15)
   • In this scenario, I'm not sure if it is possible to get num2date to return objects of type netcdftime.DatetimeGregorian. Therefore, regardless of whether the dates are of type datetime.datetime or netcdftime.DatetimeGregorian, we encode them using the 'standard' calendar type.

The rationale behind this more complicated logic has to do with how it appears netcdftime.num2date behaves. Here are some minimal examples:

from netcdftime import date2num, num2date, DatetimeGregorian>>> from datetime import datetime units = 'days since 1582-01-01'>>> # Case with minimum date before start of Gregorian calendar>>> arr = date2num([datetime(1582, 10, 1), datetime(1582, 10, 15)], units=units)>>> # To recover datetime.datetime objects, we need to use calendar='gregorian'>>> num2date(arr, units=units, calendar='gregorian') array([datetime.datetime(1582, 10, 1, 0, 0), datetime.datetime(1582, 10, 15, 0, 0)], dtype=object)>>> num2date(arr, units=units, calendar='standard')>>> # Using calendar='standard' results in netcdftime.DatetimeGregorian objects array([netcdftime._netcdftime.DatetimeGregorian(1582, 10, 1, 0, 0, 0, 0, -1, 1), netcdftime._netcdftime.DatetimeGregorian(1582, 10, 15, 0, 0, 0, 0, -1, 1)], dtype=object)

Case with minimum date after start of Gregorian calendar>>> arr = date2num([datetime(1582, 10, 15), datetime(1582, 10, 16)], units=units)>>> # Regardless of the calendar type used for decoding, we get datetime.datetime objects>>> num2date(arr, units=units, calendar='gregorian')

array([datetime.datetime(1582, 10, 15, 0, 0), datetime.datetime(1582, 10, 16, 0, 0)], dtype=object)>>> num2date(arr, units=units, calendar='standard') array([datetime.datetime(1582, 10, 15, 0, 0), datetime.datetime(1582, 10, 16, 0, 0)], dtype=object)

Given the fact that it does not seem possible to decode times after the start of the Gregorian calendar into dates of type netcdftime.DatetimeGregorian it is surprising that test_roundtrip_netcdftime_datetime_data_post_gregorian https://github.com/spencerkclark/xarray/blob/257f08607c3b0cb975a5114948d2f95f941543db/xarray/tests/test_backends.py#L383-L408 passes for DatetimeGregorian objects. It turns out it works, because timedelta arithmetic is valid between DatetimeGregorian and datetime.datetime objects for dates after the start of the Gregorian calendar:

DatetimeGregorian(1, 1, 1) - datetime(1, 1, 1) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "netcdftime/_netcdftime.pyx", line 1624, in netcdftime._netcdftime.datetime.__sub__ValueError: cannot compute the time difference between dates with different calendars>>> DatetimeGregorian(1582, 10, 30) - datetime(1582, 10, 16) datetime.timedelta(14)

(though maybe I should add an assert statement to check the date types, which would cause it to fail as currently constructed).

@jswhit https://github.com/jswhit, @jhamman https://github.com/jhamman -- is this behavior expected for netcdftime? Is there a recommended robust way for determining the calendar type for roundtripping between date2num and num2date, or is what I have in times.py#L248-L268 https://github.com/spencerkclark/xarray/blob/257f08607c3b0cb975a5114948d2f95f941543db/xarray/coding/times.py#L248-L268 sufficient?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/1252#issuecomment-374711420, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1vmkPkfyIf8NHPw_RLJxC_L8DlEEks5tgUz9gaJpZM4L3tsR .

Can we potentially merge this behind a flag for now, e.g., you only get the new behavior if you explicitly set an option, e.g., xr.set_options(enable_netcdftimeindex=True)?

There's something to be said for getting code merged when it's ready, regardless of whether we are currently issuing a breaking release. This would also let the new index get some testing in before we turn it on by default.

I think your test failures may actually be related to https://github.com/pydata/xarray/pull/1356 in xarray

I'm happy to defer to whatever non-standard calendar users think about be most useful. Possibly @jhamman has an opinion here.

Pending more cleanup on the NetCDFTimeIndex portion (both the index and its tests) are we ready to start thinking about how to include this in xarray's existing logic for decoding and encoding dates?

Yes, this seems about right to me. We'll want to save this for a major release (v0.10), since it will be backwards incompatible.

Okay this seems like a netcdftime bug. Can you report this upstream? On Sat, Feb 11, 2017 at 4:18 PM Spencer Clark notifications@github.com wrote:

@spencerkclark commented on this pull request.

In xarray/conventions/netcdftimeindex.py https://github.com/pydata/xarray/pull/1252:

@@ -120,23 +118,31 @@ def get_date_type(self): return type(self._data[0])

-def assert_all_same_netcdftime_datetimes(data): - from netcdftime._netcdftime import datetime +def assert_all_valid_date_type(data): + from netcdftime import (

Sorry, I buried this in a comment (#1252 (comment) https://github.com/pydata/xarray/pull/1252#discussion_r100678111) above. Confusingly, netcdftime.datetime does not refer to the super class:

In [1]: from netcdftime import datetime, DatetimeAllLeap

In [2]: datetime(1, 1, 1) Out[2]: netcdftime._netcdftime.DatetimeProlepticGregorian(1, 1, 1, 0, 0, 0, 0, -1, 1)

In [3]: test = DatetimeAllLeap(1, 1, 1)

In [4]: isinstance(test, datetime) Out[4]: False

In [5]: from netcdftime._netcdftime import datetime as super_datetime

In [6]: isinstance(test, super_datetime) Out[6]: True

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/1252, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1m9Egh8sElE3KoU08DEv-TvRn7KEks5rbk_JgaJpZM4L3tsR .

Currently one can create non-sensical datetimes using netcdftime._netcdftime.datetime objects. This means one can attempt to index with an out-of-bounds string or datetime without raising an error. Could this possibly be addressed upstream?

Yes, this would be best addressed upstream in netcdftime.