Indeed that meeting played an important role here. Thank you @rabernat!

Credit also due to @rabernat for organizing the workshop in late 2016 where this effort got off the ground, and to @shoyer who sketched out an initial roadmap for the implementation at that meeting.

So excited to have this in! In aospy alone, we'll be able to get rid of 100s (1000+?) of lines of code now that CFTime is in place.

Congrats to everyone who made this happen, especially @spencerclark. This feature is going to make so many people happy!

@shoyer, @jhamman, @maxim-lian, @spencerahill many thanks for the substantial feedback, help, and encouragement here. You guys are great!

Okay, I'm going to merge now. Hopefully a few of us can stress test this a bit more prior to the next release. Thanks @spencerkclark for all the work here over the past 15 months!!!

Congrats! This is a great piece of work and will be very useful to the climate community.

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

Thanks @shoyer, those are good questions. I addressed your inline comments. Let me know if you have anything else.

What happens when you try to resample along CFTimeIndex?

Through pandas, this raises an informative TypeError:

``` In [1]: from cftime import num2date

In [2]: import numpy as np

In [3]: import xarray as xr

In [4]: xr.set_options(enable_cftimeindex=True) Out[4]: <xarray.core.options.set_options at 0x10c3a99d0>

In [5]: time = num2date(np.arange(5), units='days since 0001-01-01', calendar='noleap')

In [6]: data = xr.DataArray(np.arange(5), coords=[time], dims=['time'])

In [7]: data.resample(time='2D').mean()

TypeError Traceback (most recent call last) <ipython-input-8-577095ede89a> in <module>() ----> 1 data.resample(time='2D').mean()

/Users/spencerclark/xarray-dev/xarray/xarray/core/common.pyc in resample(self, freq, dim, how, skipna, closed, label, base, keep_attrs, **indexer) 616 resampler = self._resample_cls(self, group=group, dim=dim_name, 617 grouper=grouper, --> 618 resample_dim=RESAMPLE_DIM) 619 620 return resampler

/Users/spencerclark/xarray-dev/xarray/xarray/core/resample.pyc in init(self, args, kwargs) 128 "cannot have the same name as actual dimension " 129 "('{}')! ".format(self._resample_dim, self._dim)) --> 130 super(DataArrayResample, self).init(args, kwargs) 131 132 def apply(self, func, shortcut=False, kwargs):

/Users/spencerclark/xarray-dev/xarray/xarray/core/groupby.pyc in init(self, obj, group, squeeze, grouper, bins, cut_kwargs) 233 raise ValueError('index must be monotonic for resampling') 234 s = pd.Series(np.arange(index.size), index) --> 235 first_items = s.groupby(grouper).first() 236 full_index = first_items.index 237 if first_items.isnull().any():

//anaconda/envs/xarray-dev/lib/python2.7/site-packages/pandas/core/generic.pyc in groupby(self, by, axis, level, as_index, sort, group_keys, squeeze, kwargs) 5160 return groupby(self, by=by, axis=axis, level=level, as_index=as_index, 5161 sort=sort, group_keys=group_keys, squeeze=squeeze, -> 5162 kwargs) 5163 5164 def asfreq(self, freq, method=None, how=None, normalize=False,

//anaconda/envs/xarray-dev/lib/python2.7/site-packages/pandas/core/groupby.pyc in groupby(obj, by, kwds) 1846 raise TypeError('invalid type: %s' % type(obj)) 1847 -> 1848 return klass(obj, by, kwds) 1849 1850

//anaconda/envs/xarray-dev/lib/python2.7/site-packages/pandas/core/groupby.pyc in init(self, obj, keys, axis, level, grouper, exclusions, selection, as_index, sort, group_keys, squeeze, **kwargs) 514 level=level, 515 sort=sort, --> 516 mutated=self.mutated) 517 518 self.obj = obj

//anaconda/envs/xarray-dev/lib/python2.7/site-packages/pandas/core/groupby.pyc in _get_grouper(obj, key, axis, level, sort, mutated, validate) 2848 # a passed-in Grouper, directly convert 2849 if isinstance(key, Grouper): -> 2850 binner, grouper, obj = key._get_grouper(obj, validate=False) 2851 if key.key is None: 2852 return grouper, [], obj

//anaconda/envs/xarray-dev/lib/python2.7/site-packages/pandas/core/resample.pyc in _get_grouper(self, obj, validate) 1118 def _get_grouper(self, obj, validate=True): 1119 # create the resampler and return our binner -> 1120 r = self._get_resampler(obj) 1121 r._set_binner() 1122 return r.binner, r.grouper, r.obj

//anaconda/envs/xarray-dev/lib/python2.7/site-packages/pandas/core/resample.pyc in _get_resampler(self, obj, kind) 1114 raise TypeError("Only valid with DatetimeIndex, " 1115 "TimedeltaIndex or PeriodIndex, " -> 1116 "but got an instance of %r" % type(ax).name) 1117 1118 def _get_grouper(self, obj, validate=True):

TypeError: Only valid with DatetimeIndex, TimedeltaIndex or PeriodIndex, but got an instance of 'CFTimeIndex' ```

What happens when you try to plot a DataArray with a CFTimeIndex?

I updated things such that if cftime.datetime objects are used as a coordinate when plotting, the error message looks like: ``` In [1]: from cftime import num2date

In [2]: import numpy as np

In [3]: import xarray as xr

In [4]: xr.set_options(enable_cftimeindex=True) Out[4]: <xarray.core.options.set_options at 0x10af58850>

In [5]: time = num2date(np.arange(5), units='days since 0001-01-01', calendar='noleap')

In [6]: data = xr.DataArray(np.arange(5), coords=[time], dims=['time'])

In [7]: data.plot()

TypeError Traceback (most recent call last) <ipython-input-7-118f55e0b3d0> in <module>() ----> 1 data.plot()

/Users/spencerclark/xarray-dev/xarray/xarray/plot/plot.pyc in call(self, kwargs) 357 358 def call(self, kwargs): --> 359 return plot(self._da, **kwargs) 360 361 @functools.wraps(hist)

/Users/spencerclark/xarray-dev/xarray/xarray/plot/plot.pyc in plot(darray, row, col, col_wrap, ax, rtol, subplot_kws, kwargs) 155 kwargs['ax'] = ax 156 --> 157 return plotfunc(darray, kwargs) 158 159

/Users/spencerclark/xarray-dev/xarray/xarray/plot/plot.pyc in line(darray, args, kwargs) 258 yplt = darray.coords[ylabel] 259 --> 260 _ensure_plottable(xplt) 261 262 primitive = ax.plot(xplt, yplt, args, **kwargs)

/Users/spencerclark/xarray-dev/xarray/xarray/plot/plot.pyc in _ensure_plottable(*args) 54 if not (_valid_numpy_subdtype(np.array(x), numpy_types) or 55 _valid_other_type(np.array(x), other_types)): ---> 56 raise TypeError('Plotting requires coordinates to be numeric ' 57 'or dates of type np.datetime64 or ' 58 'datetime.datetime.')

TypeError: Plotting requires coordinates to be numeric or dates of type np.datetime64 or datetime.datetime. If `cftime.datetime` objects are the data requested to be plotted, the following error message results: In [8]: data = xr.DataArray(time, coords=[np.arange(5)], dims=['x'])

In [9]: data.plot()

NotImplementedError Traceback (most recent call last) <ipython-input-9-118f55e0b3d0> in <module>() ----> 1 data.plot()

/Users/spencerclark/xarray-dev/xarray/xarray/plot/plot.pyc in call(self, kwargs) 357 358 def call(self, kwargs): --> 359 return plot(self._da, **kwargs) 360 361 @functools.wraps(hist)

/Users/spencerclark/xarray-dev/xarray/xarray/plot/plot.pyc in plot(darray, row, col, col_wrap, ax, rtol, subplot_kws, **kwargs) 124 125 if contains_cftime_datetimes(darray): --> 126 raise NotImplementedError('Plotting arrays of cftime.datetime objects ' 127 'is currently not possible.') 128

NotImplementedError: Plotting arrays of cftime.datetime objects is currently not possible. ```

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).

This could be ready. I'm happy to address any further concerns if anyone has them.

Tests are green here now. @shoyer and @spencerkclark - are we waiting on anything else before merging?

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

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.

Thanks @shoyer, it looks like that did the trick. I addressed your recent comments; let me know if you have any further feedback.

@jhamman sure thing, see https://github.com/conda-forge/cftime-feedstock/issues/2.

@spencerkclark - mind reporting the conda issue upstream at https://github.com/conda-forge/cftime-feedstock?

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.

@ritviksahajpal I see what you mean regarding the version, e.g. this is what I get if I install via my suggestion above: ```

import xarray as xr xr.version '0.8.2+dev648.gca4d7dd' ``` That said, I don't think I would read very much into that. I've been merging the master branch continually to keep things up to date.

open_rasterio had a new argument called nodatavals intrroduced in 0.10.x. This is not available in your code.

Note that nodatavals is not an argument to open_rasterio; it is an attribute that is added to the DataArray returned by open_rasterio: https://github.com/pydata/xarray/blob/c42cbe787d530db48575d73fe7a3910b0a0e4fd8/xarray/backends/rasterio_.py#L260-L263

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.

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.

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.

Thanks for the review @shoyer! I'll try and address your comments tomorrow.

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

Right, see my comment above:

The failures in Appveyor are due to the fact that the 32-bit Python 2.7 build is not using the latest version of cftime (instead it is using 1.0.0a3). @jhamman should we expect the version 1.0.0b1 to be available on that platform?

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

@spencerkclark , one question. The xarray version from git+https://github.com/spencerkclark/xarray.git@NetCDFTimeIndex seems to be 0.8.2, because of this some things seem to be missing e.g. open_rasterio had a new argument called nodatavals intrroduced in 0.10.x. This is not available in your code. Is there a reason why your xarray version is 0.8.2?

works beautifully, thanks so much!

@spencerkclark, I would like to test your changes on some netCDF files I am processing and which need to handle data in year 2300, thank you so much for enabling it! is there a github url I can get these code changes from? I understand that they might not be absolutely final.

@ritviksahajpal by all means, give it a try! You should be able to install my branch via pip: $ pip install git+https://github.com/spencerkclark/xarray.git@NetCDFTimeIndex For the new functionality you will also need to install the latest version of the cftime library: $ conda install -c conda-forge cftime and in your code you will need to opt-in via xarray.set_options(enable_cftimeindex=True). Please let me know if you run into any issues as we work towards finishing this up.

Thanks @jhamman, I think this should be ready for a final round of reviews. cftime.datetime arrays can now be accurately round-tripped if enable_cftimeindex is set to True. I also added some needed test coverage for the use of the dt accessor on arrays with cftime.datetime objects and cleaned up the test-skipping logic in test_cftimeindex.py (I no longer use a module-level importorskip).

The failures in Appveyor are due to the fact that the 32-bit Python 2.7 build is not using the latest version of cftime (instead it is using 1.0.0a3). @jhamman should we expect the version 1.0.0b1 to be available on that platform?

ping @spencerkclark to finish this up. I just released a new version of cftime so this should be ready to wrap up.

I think this is close to being ready. I've updated things for the renaming of netcdftime to cftime, and added logic to only allow for the use of a CFTimeIndex or serialization of cftime.datetime objects when the standalone cftime package is installed.

The last piece is upstream in Unidata/cftime#39. With that we should be able to slightly modify the logic here to allow us to roundtrip all dates produced via cftime.num2date (and make all the roundtrip tests pass).

@jhamman great, I can handle those if you'd like. It's basically just updating what we did in #1920, #1929, and #1933, correct? I'll do that in separate PR, which we can merge before this one.

@spencerkclark - things are well underway fixing up cftime. We'll need some namespace changes in xarray before things will work here. Do you want to do those or should I?

I can push the name change on cftime forward over the next week.

Thanks @jhamman! Once that's done, hopefully we can sure up Unidata/netcdftime#39. I think those are the last outstanding issues here.

One clean option would be to require the new cftime module (with your change) for this functionality. That way we can rely on standard behavior here.

I will wait to implement this until the name change and Unidata/netcdftime#39 go through. With Unidata/netcdftime#39 we can make the one failing test here, test_roundtrip_netcdftime_datetime_data, pass for all cftime date types.

I can push the name change on cftime forward over the next week. We should really wrap this up.

My main concern is about the stability of the netcdftime package, which it looks like might need to be renamed? Unidata/netcdftime#36

Yes, I agree we should wait until that issue is addressed so we can adapt things accordingly.

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).

Hi all, any updates on the current status for this? This will be a big help for me as well in particular for processing daily CMIP5 netcdf files. I have been following this thread as well as the original issue and really appreciate this work. One other question: This PR doesn't allow for resampling on non-standard calendars as is, but I remember @shoyer mentioning that a workaround using pandas Grouper objects will exist. Would someone be able to explain to me how this would work? Thanks!

Thanks @shoyer, I agree that would make our life easier here. I created an issue upstream: Unidata/netcdftime#37.

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 .

It occurred to me that netcdftime.num2date can return datetime.datetime objects in certain circumstances (not just objects that subclass netcdftime.datetime). 5e1c4a8 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 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:

```python

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 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: ```python

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, @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 sufficient?

Just seeing this. I'm tied up the next couple days but would be happy to review it on Wednesday. Although I doubt I'll find anything you or @shoyer wouldn't, so feel free to merge if you'd rather not hold it up another few days.

I'm sufficiently pleased with this that I think we can merge this in more or less its current state. Given the size of the PR though, I'd like to have one more person review it in detail before it goes in. Any takers from @pydata/xarray? Maybe @spencerahill?

With these latest commits, I think the new behavior should now be completely behind the enable_netcdftimeindex option (which by default is set to False); I also added a test to make sure that calling resample on an xarray object that is indexed by a NetCDFTimeIndex raises a TypeError.

Note that because I wanted the convenience of using pytest.mark.parametrize for some tests in test_coding_times.py, I took the liberty of eliminating the class-based structure in that file. In hindsight I realize I probably could have waited to complete that job in another PR, but I got a little carried away. My apologies for adding some noise to this already large diff.

@jhamman thanks, those are good questions.

This has some breaking behavior in it (e.g. resample). Do we want a way to enable the user to choose whether or not to use this new index?

Yes, I like @shoyer's idea here, and I'll see what I can do over the next few days.

Tests. Can we make sure we have tests that cover the raised errors for all the things we know won't work (e.g. resample).

Agreed -- I'll make sure to add these as well.

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.

there are some failing tests for the zarr backend, but I think those are unrelated (I fixed some that were).

Right. Those will be fixed once I merge #1793.

@jhamman @shoyer with netcdftime now on its way to becoming a standalone package, this might be at the point where it is ready for some more serious review; there are some failing tests for the zarr backend, but I think those are unrelated (I fixed some that were).

I've held off on writing a what's new entry for now, because I know that we'd like to wait for a major release to merge this (as this contains a breaking change for handling of dates with non-standard calendars).

cc @matt-long who was interested in this.

@jhamman thanks a lot for the comments and for your work towards splitting netcdftime out of netCDF4; I'll try and address them by the end of this week.

@spencerahill yes, my subsequent commits resolved the test failures associated both with the pandas update and with #1356.

On a broader note it would be great to make some more progress on this. I've noted a few specific questions above, but obviously would appreciate feedback on any aspect of these changes.

Pinging folks on this. Summer is upon us and, generally speaking for the academics among us, is a good time for projects like this.

@spencerkclark looks like you're still looking for guidance w/r/t the last batch of comments from May 10.

From the CI it also looks like I need to accommodate some updates in pandas in NetCDFTimeIndex. I will try and look into that tomorrow.

Did your subsequent commits resolve this?

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

From the CI it also looks like I need to accommodate some updates in pandas in NetCDFTimeIndex. I will try and look into that tomorrow.

1. How is this going? Could you use some help with anything?

@jhamman sure thing -- I just pushed some updates to show where I'm at.

I've made a first pass at the following: - Resetting the logic for decoding datetimes such that np.datetime64 objects are never used for non-standard calendars - Adding logic to use a NetCDFTimeIndex whenever netcdftime.datetime objects are used in an array being cast as an index (so if one reads in a Dataset from a netCDF file or creates one in Python, which is indexed by a time coordinate that uses netcdftime.datetime objects a NetCDFTimeIndex will be used rather than a generic object-based index) - Adding logic to encode netcdftime.datetime objects when saving out to netCDF files

I've added and updated some tests, but it's possible some may be redundant, and others could still be needed.

Hopefully with some guidance from you and @shoyer I can clean things up. I'll add a couple comments to the latest diff to highlight a few questions that came up as I was making these changes.

1. Pandas provides a convenience function to create arrays of datetime objects (pd.date_range). This function just passes its arguments directly on to DatetimeIndex. I'm wondering if we can provide an xr.date_range function that mimics the pandas version with an additional calendar argument?

I agree this would be quite useful. pandas' implementation is pretty complicated, but perhaps we could do something, possibly more limited, but simpler.

@spencerkclark - two questions.

1. How is this going? Could you use some help with anything?
2. Pandas provides a convenience function to create arrays of datetime objects (pd.date_range). This function just passes its arguments directly on to DatetimeIndex. I'm wondering if we can provide an xr.date_range function that mimics the pandas version with an additional calendar argument?

Thanks for jumping in @jhamman -- that is my inclination as well. I will start along this path unless anyone else has any objections.

My two cents. I think we want:

Always use netcdftime.datetime objects (and hence NetCDFTimeIndex) for representing dates with non-standard calendars

Presumably, we can address many of the negatives with this approach with follow-on PRs. The fallback logic in xarray right now for non-standard calendars is not something we want to keep and was mostly meant as a stop-gap until this feature came along.

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

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

Right. To what extent do we want to preserve the current behavior? As I understand it, what is currently done is regardless of the calendar type, every effort is made to convert decoded datetimes into np.datetime64 objects. What this means is that the only time one gets netcdftime.datetime objects in an xarray object is when either (or both):

• A date present in an array does not exist in the standard calendar (e.g. say '2000-02-30' in a '360_day' calendar).
• The dates do not fit into the range of years 1678 to 2262.

The main advantage of doing this is that it enables, wherever possible, 1D arrays of datetimes to be converted to DatetimeIndexes and all the nice things that comes with them:

1. Field accessors (for use in groupby operations)
2. Partial datetime string indexing
3. Resample for downsampling only (accurate upsampling would need to be calendar-aware)
4. "Not a time" support (i.e. support for missing values).

This practice has one disadvantage -- the calendar type of the datetimes is not preserved. Therefore if one tries to do timedelta arithmetic between values in the array, (e.g. Mar. 1st, 2001 minus Feb. 1st, 2001) one might get an inaccurate answer depending on what the original calendar type was (as was noted when this was originally implemented, and to be fair, I don't think timedelta arithmetic was even possible on netcdftime.datetime objects back then).

Options

I can think of two ways to proceed in integrating NetCDFTimeIndex into xarray:

• Only use a NetCDFTimeIndex where a DatetimeIndexcurrently cannot be used
• Always use netcdftime.datetime objects (and hence NetCDFTimeIndex) for representing dates with non-standard calendars

Tradeoffs

Only use a NetCDFTimeIndex where a DatetimeIndexcurrently cannot be used

This is perhaps the the least dramatic change one could make. It would involve not modifying the decoding logic at all (i.e. continuing to be aggressive in attempting to convert netcdftime.datetime objects to np.datetime64 types) and only using a NetCDFTimeIndex when dates were DatetimeIndex incompatible (in other words only when an array of netcdftime.datetime objects arrived at utils.safe_cast_to_index under the existing decoding logic). The timedelta issue would still remain, but this route would not change the behavior for time arrays with non-standard calendars that could be cast as ordinary DatetimeIndexes (so wherever you could use resample for non-standard calendars before, you could still do so here, and of course wherever you couldn't before, you still couldn't).

Always use netcdftime.datetime objects (and hence NetCDFTimeIndex) for representing dates with non-standard calendars

As has been discussed, the current implementation of NetCDFTimeIndex enables (1) and (2), as well as eliminates the timedelta problem, but it does not enable (3) or (4). Therefore, for instance, for those who used xarray to downsample arrays indexed by datetimes with a non-standard calendar, whose data did not violate the two bulleted specifications at the top, deciding to unilaterally use netcdftime.datetime objects for all non-standard calendar types would be a regression (though it would preserve the original calendar type).

I suppose this comes down to weighing the importance of addressing the timedelta issue (perhaps more generally preserving calendar types) versus preserving existing behavior that allows (3) and (4) for some cases with non-standard calendars.

Is this an accurate summary of the considerations we should make here? What are folks' opinions on these tradeoffs? What might be the preferred route to take?

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.

Sorry for letting this slip these last couple months — I should have time to make some more meaningful progress on this over the next few weeks. @shoyer when you get a chance, could you let me know what you think the next important steps are for this PR? 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?

As a side note, I have an initial attempt at addressing Unidata/netcdftime#8 and Unidata/netcdftime#9 in Unidata/netcdftime#12.

@spencerahill many thanks for looking into this further. I agree that simulations of tens of thousands of years are beginning to appear on the research landscape, and shouldn't necessarily be a use-case we dismiss.

I got some more concrete information c.f. on my previous comment on negative and/or 5 digit dates. The TRACE simulation outputs netCDF files uses units of thousands of years relative to 1950, and therefore doesn't use 5 integers. But it does use negative and positive floats...negative for <1950, positive for >1950.

I think it's important to note that it doesn't matter how the date information is stored in the original files; what matters is what it looks like once it's decoded. If someone analyzing this simulation would like to use a NetCDFTimeIndex, it would require that they decode these floats into actual datetimes (which would need five-digit years to express). So they would need support for five digit (possibly negative) years in partial datetime string indexing. As a side note though, this decoding is currently not possible with netCDF4.num2date (since "thousands of years" is not an accepted unit there), but I could see a way someone could do the decoding manually.

I'll need to think more about options for date parsing; ideally I would like there to be at least one common string format that one could use to specify dates for both a NetCDFTimeIndex and a DatetimeIndex (with the ISO8601 parser we have now, supporting only 4-digit, positive years, I believe that is currently the case). If we can achieve that while adding support for five-digit and negative years, that would be great, but perhaps it's best to leave that for a future PR.

@spencerkclark @shoyer I got some more concrete information c.f. on my previous comment on negative and/or 5 digit dates. The TRACE simulation outputs netCDF files uses units of thousands of years relative to 1950, and therefore doesn't use 5 integers. But it does use negative and positive floats...negative for <1950, positive for >1950.

Re: 5 digits, there is growing research interest in very long climate model integrations, e.g. http://www.longrunmip.org/, but even those for now appear <10k yr in duration.

But there are also so called EMICs (Earth Models of Intermediate Complexity) that are cheap to run for 1000s of years. Although I couldn't immediately find any published results using them for >10k yr duration...

So ultimately I'd say there definitely exists a use-case for negative times (albeit with odd format) and there likely exists a use-case for 5 digit years. IMHO these are not must-haves for the initial netcdftime implementation but should at least be kept in mind, i.e. code design that makes them not-overly-difficult to introduce eventually.

I suspect there are xarray users with more direct experience with these cases...feel free to chime in

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 .

@shoyer when you get a chance, things are ready for another review. I think the AppVeyor issues may be due to the version of netCDF4 used. Should we switch to the conda-forge channel to set up the environment there?

@shoyer thanks for your initial review comments. I'll try and push an update in the next few days.

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.

That looks awesome! Quite a turn around there.

Yes, good point on the date_type. Let me think for a bit on the best way

@MaximilianR 6496458 contains an updated version of the file containing the tests, updated to use pytest. I ended up using the ids keyword in places to clean up the test names that are output when running the tests in verbose mode, but I agree it's not super necessary.

Overall pytest seems to clean things up pretty nicely. One problem that I wish I had a better solution for happens when I am testing indexing operations in DataArrays, Series, and DataFrames. There are multiple ways of getting the same answer, which makes these tests a good candidate for using pytest.mark.parametrize; however, one of those ways, using netcdftime._netcdftime.datetime objects directly, depends on the date_type used.

For instance, it would be great if I could write something like: python @pytest.mark.parametrize('sel_arg', [ '0001', slice('0001-01-01', '0001-12-30'), [True, True, False, False], slice(date_type(1, 1, 1), date_type(1, 12, 30)), [date_type(1, 1, 1), date_type(1, 2, 1)] ], ids=['string', 'string-slice', 'bool-list', 'date-slice', 'date-list']) def test_sel(da, index, sel_arg): expected = xr.DataArray([1, 2], coords=[index[:2]], dims=['time']) result = da.sel(time=sel_arg) assert_identical(result, expected)

But I can't use date_type, which is a fixture in my current setup, in an argument to parametrize. Right now I've worked around this by resorting back to manually iterating over the cases by writing separate methods, but that's pretty verbose; might you happen to know of a cleaner way of setting things up in this case?

In any event, when you get a chance, please let me know if you have any comments / suggestions on my latest push. Thanks again for your help.

Not far off, although no need to use things like ids (or even indirect, although you can if you want for that).

More than happy to offer any guidance on tests if helpful - post an example. pytest is really nice, even if it takes a bit of time to get used to

@MaximilianR I think I'm getting the hang of it; ignore the above. I'll push a new update to the PR in a bit.

Thanks for the quick feedback on the tests @MaximilianR. Is this on the right track for doing things a little more idiomatically with pytest?

```python import pytest

from xarray.tests import assert_array_equal

def netcdftime_date_types(): pytest.importorskip('netCDF4')

from netcdftime import (
    DatetimeNoLeap, DatetimeJulian, DatetimeAllLeap,
    DatetimeGregorian, DatetimeProlepticGregorian, Datetime360Day)
return [DatetimeNoLeap, DatetimeJulian, DatetimeAllLeap,
        DatetimeGregorian, DatetimeProlepticGregorian, Datetime360Day]

@pytest.fixture(params=[]) def index(request): from xarray.core.netcdftimeindex import NetCDFTimeIndex

date_type = request.param
dates = [date_type(1, 1, 1), date_type(1, 2, 1),
         date_type(2, 1, 1), date_type(2, 2, 1)]
return NetCDFTimeIndex(dates)

@pytest.mark.parametrize('index', netcdftime_date_types(), indirect=True) @pytest.mark.parametrize(('field', 'expected'), [ ('year', [1, 1, 2, 2]), ('month', [1, 2, 1, 2]), ('day', [1, 1, 1, 1]), ('hour', [0, 0, 0, 0]), ('minute', [0, 0, 0, 0]), ('second', [0, 0, 0, 0]), ('microsecond', [0, 0, 0, 0]) ], ids=['year', 'month', 'day', 'hour', 'minute', 'second', 'microsecond']) def test_netcdftimeindex_field_accessors(index, field, expected): result = getattr(index, field) assert_array_equal(result, expected) ```