WIP Add a CFTimeIndex-enabled xr.cftime_range function #2301

spencerkclark · 2018-07-19T16:04:10Z

Closes add CFTimeIndex enabled date_range function #2142
Tests added (for all bug fixes or enhancements)
Tests passed (for all non-documentation changes)
Fully documented, including whats-new.rst for all changes and api.rst for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

I took the approach first discussed here by @shoyer and followed pandas by creating simplified offset classes for use with cftime objects to implement a CFTimeIndex-enabled cftime_range function. I still may clean things up a bit and add a few more tests, but I wanted to post this in its current state to show some progress, as I think it is more or less working. I will try to ping folks when it is ready for a more detailed review.

Here are a few examples:

In [1]: import xarray as xr

In [2]: xr.cftime_range('2000-02-01', '2002-05-05', freq='3M', calendar='noleap')
Out[2]:
CFTimeIndex([2000-02-28 00:00:00, 2000-05-31 00:00:00, 2000-08-31 00:00:00,
             2000-11-30 00:00:00, 2001-02-28 00:00:00, 2001-05-31 00:00:00,
             2001-08-31 00:00:00, 2001-11-30 00:00:00, 2002-02-28 00:00:00],
            dtype='object')

In [3]: xr.cftime_range('2000-02-01', periods=4, freq='3A-JUN', calendar='noleap')
Out[3]:
CFTimeIndex([2000-06-30 00:00:00, 2003-06-30 00:00:00, 2006-06-30 00:00:00,
             2009-06-30 00:00:00],
            dtype='object')

In [4]: xr.cftime_range(end='2000-02-01', periods=4, freq='3A-JUN')
Out[4]:
CFTimeIndex([1990-06-30 00:00:00, 1993-06-30 00:00:00, 1996-06-30 00:00:00,
             1999-06-30 00:00:00],
            dtype='object')

Hopefully the offset classes defined here would also be useful for implementing things like resample for CFTimeIndex objects (#2191) and CFTimeIndex.shift (#2244).

stickler-ci · 2018-07-19T16:04:58Z

xarray/coding/cftime_offsets.py

+
+
+def _adjust_n_years(other, n, month, reference_day):
+    """Adjust the number of times an annual offset is applied based on 


W291 trailing whitespace

xarray/coding/cftime_offsets.py

stickler-ci · 2018-07-21T17:49:21Z

xarray/coding/cftime_offsets.py

+    +----------------------+----------------------------------------------------------------+
+    | all_leap, 366_day    | ``cftime.DatetimeAllLeap``                                     |
+    +----------------------+----------------------------------------------------------------+
+    | 360_day              | ``cftime.Datetime360Day``                                      |


E501 line too long (93 > 79 characters)

stickler-ci · 2018-07-21T17:49:21Z

xarray/coding/cftime_offsets.py

+    +----------------------+----------------------------------------------------------------+
+    | 360_day              | ``cftime.Datetime360Day``                                      |
+    +----------------------+----------------------------------------------------------------+
+    | julian               | ``cftime.DatetimeJulian``                                      |


E501 line too long (93 > 79 characters)

stickler-ci · 2018-07-21T17:49:21Z

xarray/coding/cftime_offsets.py

+
+    >>> xr.date_range(start='2000', periods=6, freq='2MS', calendar='noleap')
+    CFTimeIndex([2000-01-01 00:00:00, 2000-03-01 00:00:00, 2000-05-01 00:00:00,
+                 2000-07-01 00:00:00, 2000-09-01 00:00:00, 2000-11-01 00:00:00],


E501 line too long (80 > 79 characters)

stickler-ci · 2018-07-21T17:49:21Z

xarray/coding/cftime_offsets.py

+
+    >>> xr.date_range(start='0001', periods=6, freq='2MS', calendar='standard')
+    CFTimeIndex([0001-01-01 00:00:00, 0001-03-01 00:00:00, 0001-05-01 00:00:00,
+                 0001-07-01 00:00:00, 0001-09-01 00:00:00, 0001-11-01 00:00:00],


E501 line too long (80 > 79 characters)

stickler-ci · 2018-07-21T17:49:21Z

xarray/coding/cftime_offsets.py

+    As in the standard pandas function, three of the ``start``, ``end``,
+    ``periods``, or ``freq`` arguments must be specified at a given time, with
+    the other set to ``None``.  See the `pandas documentation
+    <https://pandas.pydata.org/pandas-docs/stable/generated/pandas.date_range.html#pandas.date_range>`_ 


W291 trailing whitespace

Add docstring for xr.date_range Fix failing test Fix test skipping logic Coerce result of zip to a list in test setup Add and clean up tests Fix skip logic Skip roll_forward and roll_backward tests if cftime is not installed Expose all possible arguments to pd.date_range Add more detail to docstrings flake8 Add a what's new entry Add a short example to time-series.rst

spencerkclark · 2018-07-22T20:28:09Z

@jhamman @shoyer when you get a chance, I think this is ready for review.

I did a few more things since first pushing this PR:

Previously CFTimeIndex would raise an error if it contained an empty array; I'm not really sure what the use case would be, but to be consistent with the way DatetimeIndex behaves, I made modifications to allow for this (in some cases date_range returns an empty index). I also added the ability to give a CFTimeIndex a name.
The repr of CFTimeIndex previously provided no information regarding the calendar type of the index; I found this somewhat inconvenient, so I added a custom __unicode__ method to CFTimeIndex, which pandas.Index uses to build its repr. I think the result looks decent (I updated the examples above), but the implementation is a bit crude (relying on a number of private API functions of the base Index class). Maybe there is a better way or maybe we should put this off until later?
I added arguments to the constructor for CFTimeIndex that makes it more analogous to the constructor for a DatetimeIndex, allowing one to either pass dates directly, or create dates using arguments one can pass to date_range (inspired by Exposing/documenting CFTimeIndex as public API #2140 (comment)).

I hope the general approach seems reasonable, though if you had something else in mind for how to implement this originally, I'd be open to changing things. I tried to keep things as basic as I could (the pandas date_range function is fairly involved!).

shoyer · 2018-07-23T14:58:22Z

I haven't had the chance to look in detail at this yet, but one small point I would suggest is renaming it from xarray.date_time to xarray.cftime_range. This makes it more obvious that the function is really for users of cftime.

jhamman · 2018-08-29T23:07:18Z

@spencerkclark - sync this branch with master and we'll get this all wrapped up.

spencerkclark · 2018-08-30T19:20:11Z

Thanks @jhamman! This should be synced up and ready for review. The test failure under the dask-dev build doesn't appear to be related.

shoyer · 2018-08-31T16:25:29Z

if you merge in master again, the test suite should be passing after #2393

spencerkclark · 2018-09-01T13:15:34Z

Awesome, thanks @shoyer.

jhamman · 2018-09-11T17:57:44Z

I took a quick look at this today. It looks quite complete and I'm eager to get this in to master. I'm personally having trouble breaking off enough time to give it a full review. If anyone else in @pydata/xarray has some time to give this a close look, I'm sure @spencerkclark would really appreciate it.

shoyer · 2018-09-14T01:04:15Z

xarray/coding/cftime_offsets.py

+        elif type(other) == type(self):
+            return type(self)(self.n - other.n)
+        else:
+            raise NotImplementedError


Use return NotImplemented here (which Python will reraise as TypeError)

shoyer · 2018-09-14T01:05:55Z

xarray/coding/cftime_offsets.py

+    _freq = None
+
+    def __init__(self, n=1):
+        self.n = n


do you want to add checks here to verify that n is an integer? or maybe floats are OK, too?

Indeed n must be an integer. I added a check here. I also added a check on the month argument provided to the YearOffset classes.

shoyer · 2018-09-14T01:08:46Z

xarray/coding/cftime_offsets.py

+    elif isinstance(date_str_or_date, cftime.datetime):
+        return date_str_or_date
+    else:
+        raise ValueError('date_str_or_date must be a string or a '


should be TypeError

shoyer · 2018-09-14T01:12:04Z

xarray/coding/cftime_offsets.py

+
+    For dates from standard calendars within the ``pandas.Timestamp``-valid
+    range, this function operates as a thin wrapper around
+    ``pandas.date_range``.


I don't think this use-case is important to support, and interfaces that can return different types can be difficult to understand. I would instead always return a CFTimeIndex.

shoyer · 2018-09-14T01:15:21Z

xarray/coding/cftimeindex.py

@@ -149,10 +182,22 @@ class CFTimeIndex(pd.Index):
                                  'The microseconds of the datetime')
    date_type = property(get_date_type)

-    def __new__(cls, data):


Let's keep the constructor the same here, rather than overriding it to match the signature of cftime_range. (I know this is a deviation from pandas, but frankly I think the pandas behavior is a confusing mistake.)

Sure thing -- I retained my addition of the optional name argument so that we can support it in cftime_range (and obviously the constructor here as well).

shoyer · 2018-09-14T01:16:14Z

xarray/coding/cftimeindex.py

+        else:
+            attrs.append(('calendar', repr(infer_calendar_name(self._data))))
+
+        prepr = (pd.compat.u(",%s") %


you can just use u",%s" rather than pd.compat.u (which is only needed for old versions of Python 3 that we no longer support)

shoyer · 2018-09-14T01:17:41Z

xarray/coding/cftimeindex.py

+        Adapted from pandas.core.indexes.base.__unicode__
+        """
+        klass = self.__class__.__name__
+        data = self._format_data()


This is a little dangerous: we're using private methods here which might go away without notice. I have been burned by this in the past (specifically in xarray with pandas).

The other reason to hold off on copying the pandas repr() is that we don't implement casting like pd.to_datetime() instead the CFTimeIndex constructor, e.g.,

In [2]: pd.DatetimeIndex(['2000', '2001']) Out[2]: DatetimeIndex(['2000-01-01', '2001-01-01'], dtype='datetime64[ns]', freq=None)

(to be clear, I don't think this is a good idea either, but at least it ensures that the repr roundtrips faithfully)

Yes, admittedly this was a bit of a hack. For now I reverted things back to simply use the default repr, which CFTimeIndex inherits from pd.Index. Eventually it would be nice if the repr for CFTimeIndex indicated its calendar type, but I'll make a separate issue for that, which we can handle later in a cleaner way.

shoyer · 2018-09-14T01:19:27Z

xarray/tests/test_cftime_offsets.py

+    to_offset, get_date_type, _MONTH_ABBREVIATIONS, _cftime_range,
+    to_cftime_datetime, cftime_range)
+from xarray import CFTimeIndex
+from . import has_cftime


Instead of checking has_cftime and skipif decorators on every function, use cftime = pytest.importorskip('cftime').

shoyer · 2018-09-14T01:25:37Z

xarray/tests/test_cftime_offsets.py

+     ('2000', None, 5, 'A', 'foo'),
+     ('2000', '1999', None, 'A', 'foo')]
+)
+def test_cftime_range(start, end, periods, freq, name, calendar):


Can you add selection of calendar specific tests, e.g., verify that generating generating a year's worth of dates with noleap, all_leap and 360_day does the right thing?

shoyer · 2018-09-14T01:29:47Z

xarray/coding/cftime_offsets.py

@@ -0,0 +1,765 @@
+"""Time offset classes for use with cftime.datetime objects"""


Assuming that most of this file is copied/adapted from pandas, please add copy of the pandas copyright notice at the top.

I added a copy of the license for pandas to the top of the module. Is that what you meant? Should we do the same in cftimeindex.py?

I added a copy of the license for pandas to the top of the module. Is that what you meant?

Yes, this is what I had in mind. A comment would also be OK rather than the docstring.

Should we do the same in cftimeindex.py?

Sure, this is probably a good idea

stickler-ci · 2018-09-14T16:17:27Z

xarray/coding/cftimeindex.py

@@ -7,6 +7,7 @@

 from xarray.core import pycompat
 from xarray.core.utils import is_scalar
+from .times import infer_calendar_name


F401 '.times.infer_calendar_name' imported but unused

stickler-ci · 2018-09-14T16:17:27Z

xarray/tests/test_cftimeindex.py

 import pandas as pd
 import xarray as xr

 from datetime import timedelta
 from xarray.coding.cftimeindex import (
    parse_iso8601, CFTimeIndex, assert_all_valid_date_type,
    _parsed_string_to_bounds, _parse_iso8601_with_reso)
+from xarray.coding.cftime_offsets import get_date_type, YearBegin


F401 'xarray.coding.cftime_offsets.get_date_type' imported but unused
F401 'xarray.coding.cftime_offsets.YearBegin' imported but unused

stickler-ci · 2018-09-14T16:17:27Z

xarray/tests/test_cftimeindex.py

 import pandas as pd
 import xarray as xr

 from datetime import timedelta
 from xarray.coding.cftimeindex import (
    parse_iso8601, CFTimeIndex, assert_all_valid_date_type,
    _parsed_string_to_bounds, _parse_iso8601_with_reso)
+from xarray.coding.cftime_offsets import get_date_type, YearBegin
+from xarray.coding.times import infer_calendar_name


F401 'xarray.coding.times.infer_calendar_name' imported but unused

shoyer · 2018-09-14T16:31:02Z

xarray/coding/cftime_offsets.py

@@ -240,7 +287,7 @@ def __sub__(self, other):
        elif type(other) == type(self) and other.month == self.month:
            return type(self)(self.n - other.n, month=self.month)
        else:
-            raise NotImplementedError
+            raise NotImplemented


should be return NotImplemented, not raise NotImplemented

spencerkclark · 2018-09-14T19:56:14Z

Many thanks for the initial review @shoyer. I think I got to everything so far. I agree restricting cftime_range to only return CFTimeIndexes makes things much simpler to reason about.

shoyer · 2018-09-15T01:19:24Z

xarray/coding/cftime_offsets.py

+        return -self + other
+
+    def __apply__(self):
+        raise NotImplementedError


should also be return NotImplemented to differ implementation to subclasses

shoyer · 2018-09-19T20:24:51Z

thanks @spencerkclark !

stickler-ci reviewed Jul 19, 2018

View reviewed changes

stickler-ci reviewed Jul 21, 2018

View reviewed changes

spencerkclark force-pushed the date-range branch from 70f3cd0 to 009fbe3 Compare July 22, 2018 00:21

spencerkclark added 3 commits July 22, 2018 08:07

Allow empty CFTimeIndexes; add calendar to CFTimeIndex repr

f66ee5c

Enable CFTimeIndex constructor to optionally take date_range arguments

4291e5a

Simplify date_range to use new CFTimeIndex constructor

5c6abfc

spencerkclark added 2 commits August 3, 2018 11:55

Rename xr.date_range to xr.cftime_range

ae9e55c

Merge branch 'master' into date-range

c702ff5

spencerkclark changed the title ~~WIP Add a CFTimeIndex-enabled xr.date_range function~~ WIP Add a CFTimeIndex-enabled xr.cftime_range function Aug 3, 2018

Follow pandas behavior/naming for rollforward, rollback, and onOffset

d96f82d

spencerkclark added 2 commits August 30, 2018 14:24

Merge branch 'master' into date-range

953154a

Update docstring

30d6d92

Merge branch 'master' into date-range

058211f

shoyer reviewed Sep 14, 2018

View reviewed changes

spencerkclark added 7 commits September 14, 2018 08:25

Add pandas copyright notice to cftime_offsets.py

059cd42

Check validity of offset constructor arguments

0ff8299

Fix TypeError versus ValueError uses

d9a07cc

Use a module-level importorskip in test_cftime_offsets.py

663c7c6

Only return a CFTimeIndex from cftime_range

3e2743e

Keep CFTimeIndex constructor simple

cbb52dc

Add some explicitly calendar-specific tests

d14a60b

Revert back to default repr

885135a

stickler-ci reviewed Sep 14, 2018

View reviewed changes

lint

cc1e45d

shoyer reviewed Sep 14, 2018

View reviewed changes

spencerkclark added 3 commits September 14, 2018 14:34

return NotImplemented

04bc5f6

Convert pandas copyright notices to comments

abd53b0

test_calendar_leap_year_length -> test_calendar_year_length

6802471

shoyer approved these changes Sep 15, 2018

View reviewed changes

Use return NotImplemented in __apply__ too

19c1dfe

shoyer merged commit 5b87b6e into pydata:master Sep 19, 2018

spencerkclark mentioned this pull request Sep 25, 2018

xarray potential inconstistencies with cftime #2437

Closed

spencerkclark mentioned this pull request Jan 22, 2019

CFTimeIndex Resampling #2593

Merged

3 tasks

spencerkclark mentioned this pull request Apr 16, 2021

Calendar utilities #5155

Closed

spencerkclark mentioned this pull request Aug 2, 2024

cftime: Fix resampling when time contains 0001-01-01 #9116

Merged

3 tasks



		def _adjust_n_years(other, n, month, reference_day):
		"""Adjust the number of times an annual offset is applied based on

		@@ -0,0 +1,765 @@
		"""Time offset classes for use with cftime.datetime objects"""

WIP Add a CFTimeIndex-enabled xr.cftime_range function #2301

WIP Add a CFTimeIndex-enabled xr.cftime_range function #2301

Conversation

spencerkclark commented Jul 19, 2018 • edited Loading

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

spencerkclark commented Jul 22, 2018

shoyer commented Jul 23, 2018

jhamman commented Aug 29, 2018

spencerkclark commented Aug 30, 2018

shoyer commented Aug 31, 2018

spencerkclark commented Sep 1, 2018

jhamman commented Sep 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerkclark commented Sep 14, 2018

Choose a reason for hiding this comment

shoyer commented Sep 19, 2018

spencerkclark commented Jul 19, 2018 •

edited

Loading