Switch enable_cftimeindex to True by default #2516

spencerkclark · 2018-10-26T15:26:31Z

As discussed in #2437 and #2505, this sets the option enable_cftimeindex to True by default.

Fully documented, including whats-new.rst for all changes.

pep8speaks · 2018-10-26T15:26:35Z

Hello @spencerkclark! Thanks for updating the PR.

There are no PEP8 issues in the file xarray/coding/cftimeindex.py !
There are no PEP8 issues in the file xarray/coding/times.py !
There are no PEP8 issues in the file xarray/core/common.py !
There are no PEP8 issues in the file xarray/core/options.py !
There are no PEP8 issues in the file xarray/core/utils.py !
There are no PEP8 issues in the file xarray/plot/plot.py !
There are no PEP8 issues in the file xarray/tests/test_backends.py !
There are no PEP8 issues in the file xarray/tests/test_cftimeindex.py !
There are no PEP8 issues in the file xarray/tests/test_coding_times.py !
There are no PEP8 issues in the file xarray/tests/test_dataarray.py !
There are no PEP8 issues in the file xarray/tests/test_options.py !
There are no PEP8 issues in the file xarray/tests/test_utils.py !

Comment last updated on October 27, 2018 at 18:13 Hours UTC

dcherian · 2018-10-26T17:32:55Z

Should we add nice error messages to resample and plot telling the user what to do if they try those two with cftimeindex?

shoyer · 2018-10-26T17:47:21Z

doc/time-series.rst

@@ -223,16 +224,26 @@ Through the standalone ``cftime`` library and a custom subclass of
 functionality enabled through the standard :py:class:`pandas.DatetimeIndex` for
 dates from non-standard calendars or dates using a standard calendar, but


Can we clarify "non-standard calendars" -> "non-standard calendars commonly used in climate science"

This paragraph is missing a bit of context for non-climate users :)

shoyer · 2018-10-26T17:47:52Z

doc/time-series.rst

+
+   - Resampling along the time dimension for data indexed by a
+     :py:class:`~xarray.CFTimeIndex`
+   - Built-in plotting of data with :py:class:`cftime.datetime` coordinate axes.


It would be great to link to the GitHub issues for these.

shoyer · 2018-10-26T17:49:29Z

doc/time-series.rst

+
+   If at any time you would like to restore the old default behavior, which was
+   to attempt to decode datetimes into ``np.datetime64[ns]`` objects whenever
+   possible (regardless of calendar type), you can set ``enable_cftimeindex`` to


I would rather encourage people to convert their data to a standard calendar via normal xarray/cftime APIs. That's a cleaner solution than setting global configuration.

Can we add an example of this with existing APIs? Or maybe we should augment CFTimeIndex with a new to_datetime64 method?

I would rather encourage people to convert their data to a standard calendar via normal xarray/cftime APIs. That's a cleaner solution than setting global configuration.

This is a really good point and would make things a lot easier to understand. Currently we use xarray.coding.times.cftime_to_nptime internally; I can convert it to a better-documented public function (perhaps with friendlier error handling too).

shoyer · 2018-10-26T17:51:40Z

doc/whats-new.rst

@@ -33,6 +33,12 @@ v0.11.0 (unreleased)
 Breaking changes
 ~~~~~~~~~~~~~~~~

+- The option ``enable_cftimeindex`` has now been set to ``True`` by default.


We should think about eventually removing this option entirely, and making it simply a "no-op" for now (raising FutureWarning). In the long term I think we would prefer to avoid keeping around extraneous global options.

Makes sense -- that will clean up the datetime decoding logic (and tests) quite a bit.

spencerkclark · 2018-10-27T18:46:22Z

xarray/coding/times.py

@@ -291,7 +279,12 @@ def cftime_to_nptime(times):
    times = np.asarray(times)
    new = np.empty(times.shape, dtype='M8[ns]')
    for i, t in np.ndenumerate(times):
-        dt = datetime(t.year, t.month, t.day, t.hour, t.minute, t.second)
+        try:
+            dt = pd.Timestamp(t.year, t.month, t.day, t.hour, t.minute,


I switched to using pd.Timestamp here, because directly converting from datetime.datetime to np.datetime64[ns] did not raise an error for dates outside 1678 to 2262:

In [1]: from datetime import datetime; import numpy as np In [2]: np.datetime64(datetime(1, 1, 1), 'ns') Out[2]: numpy.datetime64('1754-08-30T22:43:41.128654848')

sounds good -- please note this in a comment in the code

The usual rule is that if you have to comment on your pull request to explain why you did something, you should just comment the code instead! :)

spencerkclark · 2018-10-27T18:46:56Z

xarray/coding/times.py

@@ -291,7 +279,12 @@ def cftime_to_nptime(times):
    times = np.asarray(times)
    new = np.empty(times.shape, dtype='M8[ns]')
    for i, t in np.ndenumerate(times):
-        dt = datetime(t.year, t.month, t.day, t.hour, t.minute, t.second)


Was there a reason we did not include the microsecond attribute of the datetime here previously?

Possibly microsecond is only a recent addition to cftime.datetime?

shoyer · 2018-10-28T00:17:22Z

xarray/coding/cftimeindex.py

+                'calendar, {!r}, to a pandas.DatetimeIndex, which uses dates '
+                'from the standard calendar.  This may lead to subtle errors '
+                'in operations that depend on the length of time between '
+                'dates.'.format(calendar), RuntimeWarning)


This looks great, but can we add a keyword argument for silencing the warning? Maybe index.to_datetimeindex(unsafe=True) could silence the warning?

shoyer · 2018-10-28T00:20:24Z

xarray/coding/times.py

@@ -291,7 +279,12 @@ def cftime_to_nptime(times):
    times = np.asarray(times)
    new = np.empty(times.shape, dtype='M8[ns]')
    for i, t in np.ndenumerate(times):
-        dt = datetime(t.year, t.month, t.day, t.hour, t.minute, t.second)


Possibly microsecond is only a recent addition to cftime.datetime?

shoyer · 2018-10-28T00:20:39Z

xarray/coding/times.py

@@ -291,7 +279,12 @@ def cftime_to_nptime(times):
    times = np.asarray(times)
    new = np.empty(times.shape, dtype='M8[ns]')
    for i, t in np.ndenumerate(times):
-        dt = datetime(t.year, t.month, t.day, t.hour, t.minute, t.second)
+        try:
+            dt = pd.Timestamp(t.year, t.month, t.day, t.hour, t.minute,


sounds good -- please note this in a comment in the code

The usual rule is that if you have to comment on your pull request to explain why you did something, you should just comment the code instead! :)

shoyer · 2018-10-28T00:21:47Z

xarray/core/common.py

@@ -683,6 +684,20 @@ def resample(self, freq=None, dim=None, how=None, skipna=None,
        else:
            raise TypeError("Dimension name should be a string; "
                            "was passed %r" % dim)
+
+        if isinstance(self.indexes[dim_name], CFTimeIndex):
+            raise TypeError(


How about NotImplementedError for now? We do plan to support this eventually

shoyer · 2018-10-28T00:24:38Z

xarray/core/options.py

@@ -83,6 +85,11 @@ class set_options(object):

    def __init__(self, **kwargs):
        self.old = OPTIONS.copy()
+        if ENABLE_CFTIMEINDEX in kwargs:


Can you add this as a function in SETTERS instead? That lets us keep the option specific logic outside of this class.

shoyer · 2018-10-28T00:24:56Z

xarray/core/options.py

@@ -52,7 +54,7 @@ class set_options(object):
      Default: ``'inner'``.
    - ``enable_cftimeindex``: flag to enable using a ``CFTimeIndex``
      for time indexes with non-standard calendars or dates outside the
-      Timestamp-valid range. Default: ``False``.
+      Timestamp-valid range. Default: ``True``.


I would remove this now -- no need to document a deprecated/unused option.

Conflicts: xarray/tests/test_coding_times.py

This would have failed before including the microsecond attribute of each date in cftime_to_nptime in eaa4a44.

shoyer · 2018-10-31T16:08:46Z

I don't know why this failed in one of the Appveyor builds:

================================== FAILURES ===================================
___________________________ test_enable_cftimeindex ___________________________
    def test_enable_cftimeindex():
        with pytest.raises(ValueError):
            xarray.set_options(enable_cftimeindex=None)
        with pytest.warns(FutureWarning, match='no-op'):
            with xarray.set_options(enable_cftimeindex=True):
>               assert OPTIONS['enable_cftimeindex']
E               Failed: DID NOT WARN. No warnings of type (<type 'exceptions.FutureWarning'>,) was emitted. The list of emitted warnings is: [].
xarray\tests\test_options.py:38: Failed

spencerkclark · 2018-11-01T01:17:06Z

Thanks @shoyer -- that was a bit puzzling but I think I sorted it out. I haven't done a careful diagnosis, but my hunch is that the previous build was running into this issue: pytest-dev/pytest#2917. A fix was merged a few weeks ago, but the Python 2.7 32-bit build is/was using a relatively old version of pytest (3.5 versus the most recent 3.9.3).

In the previous build, the FutureWarning for setting the enable_cftimeindex option was being emitted earlier in test_backends.py in the test_open_mfdataset_manyfiles test (before we were watching for it in the options tests). It was being raised there, because upon calling set_options.__exit__, every option (not just the options that were set in a with statement) would be reset to its global value. For example doing something like this would emit the warning (which even outside of this minor testing issue, I think is undesirable):

In [1]: import xarray

In [2]: with xarray.set_options(display_width=40):
  ...:     pass
  ...:
/Users/spencerclark/xarray-dev/xarray/xarray/core/options.py:49: FutureWarning: The enable_cftimeindex option is now a no-op and will be removed in a future version of xarray.
 FutureWarning)

In 6d08d3b I made changes to set_options to only fill the self.old dictionary with the old values for the keyword arguments specified in the constructor, and then had the __exit__ method only reset the values that were temporarily set before. The win-32 build now passes, and we no longer have the undesirable warning behavior shown above. Do you think that was a reasonable fix?

shoyer · 2018-11-01T01:22:01Z

In 6d08d3b I made changes to set_options to only fill the self.old dictionary with the old values for the keyword arguments specified in the constructor, and then had the exit method only reset the values that were temporarily set before. The win-32 build now passes, and we no longer have the undesirable warning behavior shown above. Do you think that was a reasonable fix?

This looks like a reasonable fix to me

* upstream/master: (122 commits) add missing , and article in error message (pydata#2557) Add libnetcdf, libhdf5, pydap and cfgrib to xarray.show_versions() (pydata#2555) revert to dev version for 0.11.1 Release xarray v0.11 DOC: update whatsnew for xarray 0.11 release (pydata#2548) Drop the hack needed to use CachingFileManager as we don't use it anymore. (pydata#2544) add full test env for py37 ci env (pydata#2545) Remove old-style resample example in documentation (pydata#2543) Stop loading tutorial data by default (pydata#2538) Remove the old syntax for resample. (pydata#2541) Remove use of deprecated, unused keyword. (pydata#2540) Deprecate inplace (pydata#2524) Zarr chunking (GH2300) (pydata#2487) Include multidimensional stacking groupby in docs (pydata#2493) (pydata#2536) Switch enable_cftimeindex to True by default (pydata#2516) Raise more informative error when converting tuples to Variable. (pydata#2523) Global option to always keep/discard attrs on operations (pydata#2482) Remove tests where answers change in cftime 1.0.2.1 (pydata#2522) Finish deprecation cycle for DataArray.__contains__ checking array values (pydata#2520) Fix bug where OverflowError is not being raised (pydata#2519) ...

Switch enable_cftimeindex to True by default

4a2acbc

Add a friendlier error message when plotting cftime objects

2c70d32

shoyer reviewed Oct 26, 2018

View reviewed changes

spencerkclark added 6 commits October 26, 2018 14:05

Mention that the non-standard calendars are used in climate science

69f6dc7

Add GH issue references to docs

d203491

Deprecate enable_cftimeindex option

93599ca

Add CFTimeIndex.to_datetimeindex method

fa729ec

Add friendlier error message for resample

939b953

lint

1a6841d

spencerkclark commented Oct 27, 2018

View reviewed changes

shoyer reviewed Oct 28, 2018

View reviewed changes

spencerkclark added 8 commits October 28, 2018 09:23

Address review comments

f65b764

Take into account microsecond attribute in cftime_to_nptime

eaa4a44

Merge branch 'master' into enable-cftimeindex

147eace

Conflicts: xarray/tests/test_coding_times.py

Add test for decoding dates with microsecond-resolution units

42406e9

This would have failed before including the microsecond attribute of each date in cftime_to_nptime in eaa4a44.

Fix typo in time-series.rst

359016d

Formatting

a72d41e

Merge branch 'master' into enable-cftimeindex

315b093

Fix test_decode_cf_datetime_non_iso_strings

079cfdf

Prevent warning emitted from set_options.__exit__

6d08d3b

shoyer merged commit 656f8bd into pydata:master Nov 1, 2018

spencerkclark deleted the enable-cftimeindex branch November 1, 2018 17:52

This was referenced Feb 8, 2019

xarray potential inconstistencies with cftime #2437

Closed

Add use_cftime option to open_dataset #2759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch enable_cftimeindex to True by default #2516

Switch enable_cftimeindex to True by default #2516

spencerkclark commented Oct 26, 2018 •

edited

Loading

pep8speaks commented Oct 26, 2018 •

edited

Loading

dcherian commented Oct 26, 2018

shoyer Oct 26, 2018

shoyer Oct 26, 2018

shoyer Oct 26, 2018

spencerkclark Oct 26, 2018

shoyer Oct 26, 2018

spencerkclark Oct 26, 2018

spencerkclark Oct 27, 2018

shoyer Oct 28, 2018

spencerkclark Oct 27, 2018

shoyer Oct 28, 2018

shoyer Oct 28, 2018

shoyer Oct 28, 2018

shoyer Oct 28, 2018

shoyer Oct 28, 2018

shoyer Oct 28, 2018

shoyer Oct 28, 2018

shoyer commented Oct 31, 2018

spencerkclark commented Nov 1, 2018

shoyer commented Nov 1, 2018

		@@ -223,16 +224,26 @@ Through the standalone ``cftime`` library and a custom subclass of
		functionality enabled through the standard :py:class:`pandas.DatetimeIndex` for
		dates from non-standard calendars or dates using a standard calendar, but

Switch enable_cftimeindex to True by default #2516

Switch enable_cftimeindex to True by default #2516

Conversation

spencerkclark commented Oct 26, 2018 • edited Loading

pep8speaks commented Oct 26, 2018 • edited Loading

Comment last updated on October 27, 2018 at 18:13 Hours UTC

dcherian commented Oct 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented Oct 31, 2018

spencerkclark commented Nov 1, 2018

shoyer commented Nov 1, 2018

spencerkclark commented Oct 26, 2018 •

edited

Loading

pep8speaks commented Oct 26, 2018 •

edited

Loading