Deprecate decode timedelta #2105

ocefpaf · 2018-05-04T13:50:33Z

Closes Undesired decoding to timedelta64 (was: units of "seconds" translated to time coordinate) #1621 (remove if there is no corresponding issue, which should only be the case for minor changes)
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'll add tests, docs, and the whats-new entry later if I'm on the right path here.

xref.: #843, #940, and #2085

See http://nbviewer.jupyter.org/gist/ocefpaf/e736c07faf3d4c9361ecf546a692c2cd

stickler-ci · 2018-05-04T13:50:46Z

xarray/conventions.py

@@ -293,10 +294,15 @@ def decode_cf_variable(name, var, concat_characters=True, mask_and_scale=True,
                      variables.CFScaleOffsetCoder()]:
            var = coder.decode(var, name=name)

+    enable_future_time_unit_decoding = OPTIONS['enable_future_time_unit_decoding']


E501 line too long (82 > 79 characters)

ocefpaf · 2018-05-04T13:51:46Z

xarray/coding/times.py

@@ -180,6 +180,9 @@ def decode_cf_timedelta(num_timedeltas, units):
    """Given an array of numeric timedeltas in netCDF format, convert it into a
    numpy timedelta64[ns] array.
    """
+    warnings.warn('Decoding timedeltas been deprecated and '


I'm not sure if you want to deprecate decode_cf_timedelta too or if you want to keep that around in case anyone wants to manually convert data to timedeltas. I don't have a user case for that but it was the default behavior, so maybe someone uses this.

PS: I never got any feedback from the mailing list users https://groups.google.com/forum/#!searchin/xarray/timedelta%7Csort:date/xarray/iadL57Zgw6w/KaW9UPVTCAAJ

Let's leave this function/CFTimedeltaCoder around for now and raise the warning from decode_cf_variable instead.

In the future (after we finish refactoring the backends API), users will have to explicitly pass it in as a coder to use when reading data.

Makes sense.

ocefpaf · 2018-05-04T13:53:54Z

xarray/coding/times.py

        return Variable(dims, data, attrs, encoding)

    def decode(self, variable, name=None):
+        warnings.warn('Decoding timedeltas been deprecated and '
+                      'will be removed in xarray v??.',


What version of xarray would be OK to make this default?

Let's say v0.11 for now. We can always push this back.

ocefpaf · 2018-05-04T13:54:48Z

xarray/conventions.py

@@ -293,10 +294,15 @@ def decode_cf_variable(name, var, concat_characters=True, mask_and_scale=True,
                      variables.CFScaleOffsetCoder()]:
            var = coder.decode(var, name=name)

+    enable_future_time_unit_decoding = OPTIONS['enable_future_time_unit_decoding']


An suggestions for a shorter name here?

You can split this expression across lines:

enable_future_time_unit_decoding = OPTIONS[ 'enable_future_time_unit_decoding']

I wasn't really worried about the linter but rather with the long name 😄
But naming things is hard!

shoyer

Looks like a good start! The last thing I'd like to see is support for automatically serializing/deserializing timedelta64 by saving an attribute dtype='timedelta64[ns]'. This will preserve the ability to roundtrip this data to netCDF, which I do think is useful for some users -- otherwise users will have xarray objects that they can no longer save to netCDF.

shoyer · 2018-05-04T15:52:07Z

xarray/conventions.py

@@ -293,10 +294,15 @@ def decode_cf_variable(name, var, concat_characters=True, mask_and_scale=True,
                      variables.CFScaleOffsetCoder()]:
            var = coder.decode(var, name=name)

+    enable_future_time_unit_decoding = OPTIONS['enable_future_time_unit_decoding']


You can split this expression across lines:

enable_future_time_unit_decoding = OPTIONS[ 'enable_future_time_unit_decoding']

shoyer · 2018-05-04T15:53:17Z

xarray/coding/times.py

        return Variable(dims, data, attrs, encoding)

    def decode(self, variable, name=None):
+        warnings.warn('Decoding timedeltas been deprecated and '
+                      'will be removed in xarray v??.',


Let's say v0.11 for now. We can always push this back.

shoyer · 2018-05-04T15:56:16Z

xarray/coding/times.py

@@ -180,6 +180,9 @@ def decode_cf_timedelta(num_timedeltas, units):
    """Given an array of numeric timedeltas in netCDF format, convert it into a
    numpy timedelta64[ns] array.
    """
+    warnings.warn('Decoding timedeltas been deprecated and '


Let's leave this function/CFTimedeltaCoder around for now and raise the warning from decode_cf_variable instead.

In the future (after we finish refactoring the backends API), users will have to explicitly pass it in as a coder to use when reading data.

ocefpaf · 2018-05-05T18:42:35Z

doc/io.rst

@@ -335,6 +335,10 @@ like ``'days'`` for ``timedelta64`` data. ``calendar`` should be one of the cale
 supported by netCDF4-python: 'standard', 'gregorian', 'proleptic_gregorian' 'noleap',
 '365_day', '360_day', 'julian', 'all_leap', '366_day'.

+The automatic decoding of untis like ``'days'`` to ``timedelta64`` is deprecated and
+will be removed in xarray version 0.11. The default behavior will keep the same numeric
+type of the original data instead of converting to ``timedelta64``.


@shoyer b/c english is not my mother tongue I really appreciate your help wording this part.

ocefpaf · 2018-05-05T18:44:05Z

doc/time-series.rst

-When reading or writing netCDF files, xarray automatically decodes datetime and
-timedelta arrays using `CF conventions`_ (that is, by using a ``units``
+When reading or writing netCDF files, xarray automatically decodes datetime
+arrays using `CF conventions`_ (that is, by using a ``units``


I think that removing the mention of timedelta here is better b/c the netCDF original data is rarely a timedelta. The case of wave periods, or any other phenomena 'period,' isn't a timedelta IMO.

ocefpaf · 2018-05-05T18:47:11Z

@shoyer this is ready for a second round of reviews.

The last thing I'd like to see is support for automatically serializing/deserializing timedelta64 by saving an attribute dtype='timedelta64[ns]'. This will preserve the ability to roundtrip this data to netCDF, which I do think is useful for some users -- otherwise users will have xarray objects that they can no longer save to netCDF.

I'm not sure I follow. Roundtrip is easier now since the original time unit dtype is preserved, no? Ping @rsignell-usgs who is a netCDF/CF specialist 😉

shoyer · 2018-05-07T02:23:13Z

I'm not sure I follow. Roundtrip is easier now since the original time unit dtype is preserved, no? Ping rsignell-usgs who is a netCDF/CF specialist

To clarify: I would like to be able to round-trip something like the following dataset to netCDF. The following code should still work without raising an error:

In [30]: import xarray

In [31]: import pandas as pd

In [32]: ds = xarray.Dataset({'foo': ('dt', [1, 2, 3])}, {'dt': pd.to_timedelta(['1 day', '2 days', '
    ...: 3 days'])})

In [33]: ds
Out[33]:
<xarray.Dataset>
Dimensions:  (dt: 3)
Coordinates:
  * dt       (dt) timedelta64[ns] 1 days 2 days 3 days
Data variables:
    foo      (dt) int64 1 2 3

In [34]: assert ds.identical(xarray.open_dataset(ds.to_netcdf()))

ocefpaf · 2018-05-07T12:49:14Z

I would like to be able to round-trip something like the following dataset to netCDF.

OK, I though you meant roundtrip from the netCDF file and back. In my line of work handling high level python objects serialization like that is usually not desired as the user should be responsible of how s/he wants to save the data. (I also never had an application that required timedelta, so it was hard for me to contextualize that.)

I'll be away the next weeks, so if someone wants to pick it up from here please feel free to do so. Otherwise I'll try to get back to this once I return to the office.

stickler-ci reviewed May 4, 2018

View reviewed changes

ocefpaf commented May 4, 2018

View reviewed changes

shoyer reviewed May 4, 2018

View reviewed changes

ocefpaf force-pushed the deprecate_decode_timedelta branch 2 times, most recently from 089b777 to 0e5ca37 Compare May 4, 2018 18:00

ocefpaf added 8 commits May 5, 2018 15:41

issue FutureWarnings

dbbece5

add enable_future_time_unit_decoding option

087bcd8

future behavior won't convert to timedelta

ec2bb75

review actions

adc2309

add tests for deprecation and new behavior

4966e87

whats-new entry

3e708f9

document the future behavior

2ef9d41

don't mention timedelta serialization

39eb727

ocefpaf force-pushed the deprecate_decode_timedelta branch from 4788690 to 39eb727 Compare May 5, 2018 18:41

ocefpaf commented May 5, 2018

View reviewed changes

shoyer mentioned this pull request Oct 30, 2018

Undesired automatic conversion of 'dtype' based on variable units 'days'? #2527

Closed

tlogan2000 mentioned this pull request Oct 31, 2018

xr.to_netcdf export changes 'dtype' after summer days calculation Ouranosinc/xclim#89

Closed

ocefpaf closed this May 17, 2019

ocefpaf deleted the deprecate_decode_timedelta branch May 17, 2019 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate decode timedelta #2105

Deprecate decode timedelta #2105

ocefpaf commented May 4, 2018 •

edited

Loading

stickler-ci May 4, 2018

ocefpaf May 4, 2018 •

edited

Loading

shoyer May 4, 2018

ocefpaf May 4, 2018

ocefpaf May 4, 2018

shoyer May 4, 2018

ocefpaf May 4, 2018

shoyer May 4, 2018

ocefpaf May 4, 2018

shoyer left a comment

shoyer May 4, 2018

shoyer May 4, 2018

shoyer May 4, 2018

ocefpaf May 5, 2018

ocefpaf May 5, 2018 •

edited

Loading

ocefpaf commented May 5, 2018

shoyer commented May 7, 2018

ocefpaf commented May 7, 2018

Deprecate decode timedelta #2105

Deprecate decode timedelta #2105

Conversation

ocefpaf commented May 4, 2018 • edited Loading

Choose a reason for hiding this comment

ocefpaf May 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer left a 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

ocefpaf May 5, 2018 • edited Loading

Choose a reason for hiding this comment

ocefpaf commented May 5, 2018

shoyer commented May 7, 2018

ocefpaf commented May 7, 2018

ocefpaf commented May 4, 2018 •

edited

Loading

ocefpaf May 4, 2018 •

edited

Loading

ocefpaf May 5, 2018 •

edited

Loading