fix datetime_to_numeric and Variable._to_numeric #2668

fujiisoup · 2019-01-11T22:02:07Z

Closes datetime interpolation doesn't work #2667
Tests added
Fully documented, including whats-new.rst for all changes and api.rst for new API

Started to fixing #2667

fujiisoup · 2019-01-11T22:04:05Z

xarray/core/duck_array_ops.py

+        else:
+            print(type(array), array.dtype)
+            print(array)
+            return array.astype(dtype)


Some tests pass a np.ndarray with object dtype, an entry of which is datetime.datetime, such as

<class 'numpy.ndarray'> object [datetime.timedelta(0) datetime.timedelta(days=1) datetime.timedelta(days=2)]

Is it an expected behavior?

I don't think astype() does any conversion for object arrays.

@fujiisoup this function might be helpful for casting the object arrays:

xarray/xarray/core/variable.py

Lines 132 to 136 in d4c4682

def _possibly_convert_objects(values):

"""Convert arrays of datetime.datetime and datetime.timedelta objects into

datetime64 and timedelta64, according to the pandas convention.

"""

return np.asarray(pd.Series(values.ravel())).reshape(values.shape)

fujiisoup

Still tests are failing.
Probably I do not well understand how cftime is working now...

shoyer · 2019-01-11T22:21:45Z

@spencerkclark what convention do you use for missing values in cftime arrays?

spencerkclark · 2019-01-11T22:37:06Z

what convention do you use for missing values in cftime arrays?

Currently there is nothing built in to xarray to support missing values for cftime arrays. I think we would have to come up with a convention.

shoyer · 2019-01-11T22:44:18Z

OK, in that case let's only worry about NaT for datetime64/timedelta64 arrays.

…

On Fri, Jan 11, 2019 at 2:37 PM Spencer Clark ***@***.***> wrote: what convention do you use for missing values in cftime arrays? Currently there is nothing built in to xarray to support missing values for cftime arrays. I think we would have to come up with a convention. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2668 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABKS1iSjXGAEXm6e5gqGCY390bNdMIiMks5vCRISgaJpZM4Z8P5F> .

shoyer · 2019-01-11T22:44:32Z

(at least for now, until we can figure out how to handle missing values)

…

On Fri, Jan 11, 2019 at 2:44 PM Stephan Hoyer ***@***.***> wrote: OK, in that case let's only worry about NaT for datetime64/timedelta64 arrays. On Fri, Jan 11, 2019 at 2:37 PM Spencer Clark ***@***.***> wrote: > what convention do you use for missing values in cftime arrays? > > Currently there is nothing built in to xarray to support missing values > for cftime arrays. I think we would have to come up with a convention. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#2668 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ABKS1iSjXGAEXm6e5gqGCY390bNdMIiMks5vCRISgaJpZM4Z8P5F> > . >

xarray/core/duck_array_ops.py

pep8speaks · 2019-01-13T10:09:26Z

Hello @fujiisoup! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 10, 2019 at 15:04 Hours UTC

fujiisoup · 2019-01-13T14:10:32Z

I couldn't make datetime_to_numeric compatible both np.ndarray and Variable. I think it would be cleaner if Variable has a (private) method to do this, like Variable._to_numeric(offset=None, datetime_unit=None).

shoyer · 2019-01-13T16:29:44Z

I couldn't make datetime_to_numeric compatible both np.ndarray and Variable. I think it would be cleaner if Variable has a (private) method to do this, like Variable._to_numeric(offset=None, datetime_unit=None).

Sounds good to me. It's definitely best to handle numpy arrays and xarray.Variable objects separately.

spencerkclark

Thanks @fujiisoup for your working cleaning up this logic; I have a few initial comments/questions.

spencerkclark · 2019-01-13T17:19:18Z

doc/whats-new.rst

@@ -40,6 +40,8 @@ Enhancements
 Bug fixes
 ~~~~~~~~~

+- Bug fix for interpolation with an datetime array. (:issue:`2668`)


I think @dcherian caught this bug before it made it into a release, so maybe we do not need a bug-fix note?

xarray/tests/test_duck_array_ops.py

spencerkclark · 2019-01-13T17:24:58Z

xarray/tests/test_interp.py

+
+@requires_cftime
+@requires_scipy
+def test_cftime_interp():


Do you think you can find a better name for this test? It does not seem to use cftime.

spencerkclark · 2019-01-13T17:42:50Z

xarray/core/dataset.py

@@ -24,6 +24,7 @@
 from .coordinates import (
    DatasetCoordinates, LevelCoordinatesSource,
    assert_coordinate_consistent, remap_label_indexers)
+from .duck_array_ops import datetime_to_numeric


Within differentiate, we use datetime_to_numeric on Variable objects; I think you should switch to the new Variable-specific method there.

spencerkclark · 2019-01-13T17:51:19Z

xarray/core/duck_array_ops.py

+    array = array - offset
+
+    if datetime_unit:
+        array = array / np.timedelta64(1, datetime_unit)


I think we need to move this to after we potentially convert an array of datetime.timedelta objects to np.timedelta64 (otherwise this will raise an error).

To explain this a little more -- for cftime dates, array - offset (a few lines above) will return an array of datetime.timedelta objects. When datetime_to_numeric was used with xarray.Variable objects, we did not have to worry about converting to np.timedelta64, because that would happen automatically. In the case of pure NumPy or dask arrays, we do.

fujiisoup · 2019-01-13T21:35:07Z

Thanks, @spencerkclark

It looks I need more work for this (probably a week).
It is not working for cftime stuffs, but I do not yet understand how it is handled in xarray.

spencerkclark · 2019-01-13T23:14:00Z

It looks I need more work for this (probably a week).
It is not working for cftime stuffs, but I do not yet understand how it is handled in xarray.

Thanks @fujiisoup, no worries. There was only one place where I saw a potential issue for cftime dates, which I noted above. I made a PR to your branch with the fix (fujiisoup#9); let me know if there were other places with issues and I can sort those out too.

Some minor changes to try and help with cftime dates

fujiisoup · 2019-01-14T14:11:28Z

Thanks, @spencerkclark, for the PR.
It was super helpful.

I will finish it up tonight (hopefully).

spencerkclark

This is looking pretty good @fujiisoup.

In #2667 you mentioned:

Then, it would be probably nice if datetime_to_numeric only accepts np.ndarray and da.array not Variable or DataArray, and move this function to duck_array_ops.

By da.array I'm assuming you mean dask arrays? Should we strive to have compatibility in datetime_to_numeric there? Right now for dask arrays with dtype datetime64[ns] it eagerly loads the data, while for cftime dask arrays there is an error. I think both issues would be reasonably straightforward to fix, but do you think it's worth the effort?

Again I'm happy to help out, particularly on the cftime side.

spencerkclark · 2019-01-17T14:33:17Z

xarray/tests/test_interp.py

@@ -573,3 +573,17 @@ def test_cftime_to_non_cftime_error():

    with pytest.raises(TypeError):
        da.interp(time=0.5)
+
+
+@requires_cftime


Sorry, please remove this decorator too.

# Conflicts: # xarray/core/dataset.py # xarray/core/missing.py

fujiisoup · 2019-02-10T13:44:54Z

@spencerkclark

Sorry for leaving it for a long time.
I was a little stressed these days...

Should we strive to have compatibility in datetime_to_numeric there?

As I'm not a heavy user of datetime arrays, I personally do not mind if we do not have dask compatibility for datetime_to_numeric.
Probably it would be OK to leave it for the later development if someone wants this feature.

spencerkclark · 2019-02-10T13:55:55Z

As I'm not a heavy user of datetime arrays, I personally do not mind if we do not have dask compatibility for datetime_to_numeric.

No worries @fujiisoup, this is totally fine with me. I think the most common use case for datetime arrays is in indexes, which currently need to be stored in memory anyway, so I don't think the dask use case would come up very often.

fujiisoup · 2019-02-11T09:47:24Z

Thanks @spencerkclark for reviewing. Merged.

spencerkclark · 2019-02-11T11:58:22Z

Thanks for wrapping this up @fujiisoup!

* master: typo in whats_new (pydata#2763) Update computation.py to use Python 3 function signatures (pydata#2756) add h5netcdf+dask tests (pydata#2737) Fix name loss when masking (pydata#2749) fix datetime_to_numeric and Variable._to_numeric (pydata#2668) Fix mypy errors (pydata#2753) enable internal plotting with cftime datetime (pydata#2665) remove references to cyordereddict (pydata#2750) BUG: Pass kwargs to the FileManager for pynio engine (pydata#2380) (pydata#2732) reintroduce pynio/rasterio/iris to py36 test env (pydata#2738) Fix CRS being WKT instead of PROJ.4 (pydata#2715) Refactor (part of) dataset.py to use explicit indexes (pydata#2696)

WIP: fix regression about datetime_to_numeric

9acf580

fujiisoup commented Jan 11, 2019

View reviewed changes

spencerkclark reviewed Jan 11, 2019

View reviewed changes

xarray/core/duck_array_ops.py Show resolved Hide resolved

fujiisoup added 3 commits January 12, 2019 14:33

Workaround for object array

0a2b0c6

added a whatsnew

a4fceba

rearrange tests

fca735c

lint

b262437

Added Variable._to_numeric

96422e3

spencerkclark reviewed Jan 13, 2019

View reviewed changes

Fix for cftime

a7129d1

Merge pull request #9 from spencerkclark/fix-interp-cftime

24d72bc

Some minor changes to try and help with cftime dates

fujiisoup added 2 commits January 14, 2019 21:31

Merge branch 'master' into fix_datetime_to_numeic

8c1fd0a

Update via comments

c319da6

fujiisoup changed the title ~~WIP: fix regression about datetime_to_numeric~~ fix datetime_to_numeric and Variable._to_numeric Jan 14, 2019

lint

63350ee

spencerkclark reviewed Jan 17, 2019

View reviewed changes

spencerkclark mentioned this pull request Feb 10, 2019

silence warning for decode_cf_datetime? #2754

Closed

fujiisoup added 2 commits February 10, 2019 14:35

Merge branch 'master' into fix_datetime_to_numeic

a49052c

# Conflicts: # xarray/core/dataset.py # xarray/core/missing.py

Fix via comment

003ac39

fujiisoup added 2 commits February 10, 2019 15:22

Fix errors

552aa94

lint

0b26615

spencerkclark approved these changes Feb 10, 2019

View reviewed changes

fujiisoup merged commit 4cd56a9 into pydata:master Feb 11, 2019

fujiisoup deleted the fix_datetime_to_numeic branch February 11, 2019 09:47

spencerkclark mentioned this pull request Dec 18, 2019

interp with long cftime coordinates raises an error #3641

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix datetime_to_numeric and Variable._to_numeric #2668

fix datetime_to_numeric and Variable._to_numeric #2668

fujiisoup commented Jan 11, 2019 •

edited

Loading

fujiisoup Jan 11, 2019

shoyer Jan 11, 2019

spencerkclark Jan 11, 2019

fujiisoup left a comment

shoyer commented Jan 11, 2019

spencerkclark commented Jan 11, 2019

shoyer commented Jan 11, 2019 via email

shoyer commented Jan 11, 2019 via email

pep8speaks commented Jan 13, 2019 •

edited

Loading

fujiisoup commented Jan 13, 2019

shoyer commented Jan 13, 2019

spencerkclark left a comment

spencerkclark Jan 13, 2019

spencerkclark Jan 13, 2019

spencerkclark Jan 13, 2019

spencerkclark Jan 13, 2019

spencerkclark Jan 13, 2019

fujiisoup commented Jan 13, 2019

spencerkclark commented Jan 13, 2019

fujiisoup commented Jan 14, 2019

spencerkclark left a comment

spencerkclark Jan 17, 2019

fujiisoup commented Feb 10, 2019

spencerkclark commented Feb 10, 2019

fujiisoup commented Feb 11, 2019

spencerkclark commented Feb 11, 2019

	def _possibly_convert_objects(values):
	"""Convert arrays of datetime.datetime and datetime.timedelta objects into
	datetime64 and timedelta64, according to the pandas convention.
	"""
	return np.asarray(pd.Series(values.ravel())).reshape(values.shape)

fix datetime_to_numeric and Variable._to_numeric #2668

fix datetime_to_numeric and Variable._to_numeric #2668

Conversation

fujiisoup commented Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujiisoup left a comment

Choose a reason for hiding this comment

shoyer commented Jan 11, 2019

spencerkclark commented Jan 11, 2019

shoyer commented Jan 11, 2019 via email

shoyer commented Jan 11, 2019 via email

pep8speaks commented Jan 13, 2019 • edited Loading

Comment last updated on February 10, 2019 at 15:04 Hours UTC

fujiisoup commented Jan 13, 2019

shoyer commented Jan 13, 2019

spencerkclark 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

Choose a reason for hiding this comment

fujiisoup commented Jan 13, 2019

spencerkclark commented Jan 13, 2019

fujiisoup commented Jan 14, 2019

spencerkclark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujiisoup commented Feb 10, 2019

spencerkclark commented Feb 10, 2019

fujiisoup commented Feb 11, 2019

spencerkclark commented Feb 11, 2019

fujiisoup commented Jan 11, 2019 •

edited

Loading

pep8speaks commented Jan 13, 2019 •

edited

Loading