Allow for plotting dummy netCDF4.datetime objects. #1261

willirath · 2017-02-11T22:03:33Z

Currently, xarray/plot.py raises a TypeError if the data to be plotted are not among [np.floating, np.integer, np.timedelta64, np.datetime64].

This PR adds netCDF4.datetime objects to the list of allowed data types. These occur, because xarray/conventions.py passes undecoded netCDF4.datetime objects if decoding to numpy.datetime64 fails.

shoyer · 2017-02-11T23:57:53Z

Does matplotlib actually plot netcdftime datetimes correctly?

It would be nice to have a unit test to verify this works. See xarray/tests/test_plot.py for examples.

willirath · 2017-02-12T00:01:21Z

Failing tests:

xarray/tests/test_plot.py::TestPlot1D::test_nonnumeric_index_raises_typeerror FAILED
xarray/tests/test_plot.py::TestContourf::test_nonnumeric_index_raises_typeerror FAILED
xarray/tests/test_plot.py::TestContour::test_nonnumeric_index_raises_typeerror FAILED
xarray/tests/test_plot.py::TestPcolormesh::test_nonnumeric_index_raises_typeerror FAILED
xarray/tests/test_plot.py::TestImshow::test_nonnumeric_index_raises_typeerror FAIL

All of these check strings as input to the plot functions and expect a TypeError.

xarray.plot._right_dtype, however, uses numpy.issubdtype to check, if the types of the given data are subtypes of the now included netCDF4.datetime, which is interpreted as numpy.generic. And as numpy.generic is the highest type in Numpy's type hierarchy, xarray.plot._right_dtype now always returns True.

A possible way out would be to check for numeric types and datetime-like types separately.

shoyer · 2017-02-12T00:03:17Z

xarray/plot/plot.py

@@ -32,7 +32,9 @@ def _ensure_plottable(*args):
    Raise exception if there is anything in args that can't be plotted on
    an axis.
    """
-    plottypes = [np.floating, np.integer, np.timedelta64, np.datetime64]
+    from netCDF4 import datetime as nc4datetime


This needs to be guarded behind a try/except clause, because netCDF4 is not a required dependency of xarray. Use from netcdftime import datetime instead of netCDF4 (@jhamman is moving netcdftime to its own package). Also, put import statements at the top of the file.

What should I do to prevent netcdftime.datetime to be understood as numpy.generic? Rewrite xarray.plot._ensure_plottable to handle Numpy types and netcdftime.datetime separately?

Yes, looks like we need a separate check for object dtype arrays (it's okay to loop over element).

willirath · 2017-02-12T00:03:26Z

Does matplotlib actually plot netcdftime datetimes correctly?

Yes it does. They are just treated like datetime.datetime objects.

willirath · 2017-02-12T00:26:59Z

Add tests showing that netcdftime.datetime objects can be plotted (line and 2d plots).
Rewrite xarray.plot._ensure_plottable to only let numeric and date-like types pass.

willirath · 2017-02-13T09:54:30Z

Does matplotlib actually plot netcdftime datetimes correctly?

Yes it does. They are just treated like datetime.datetime objects.

I was wrong here: netCDF4.datetime is datetime.datetime while (netcdftime.datetime is DatetimeProlepticGregorian.

And matplotlib fails on the latter: https://gist.github.com/willirath/48c04db0481e986fac0e44549e164ab0

Allow for numeric and date-like numpy data types and for datetime objects.

willirath · 2017-02-13T17:21:37Z

a85b5c6 and e31aaab add a test plotting against a coordinate with netCDF4.datetime objects.

6f5e30f adds datetime.datetime objects to the list of plottable dtypes and splits the check into numpy dtypes (using numpy's type hierarchy) and all other types.

willirath · 2017-02-13T17:50:13Z

netcdftime.datetime vs. datetime.datetime is discussed in Unidata/cftime#8. For now, I only added datetime.datetime to the list of plottable other types (see https://github.com/willirath/xarray/blob/6f5e30f3a2f8ca907f0fd91bcd571e7d98027896/xarray/plot/plot.py#L52).

shoyer

This looks good to me, though note that it's difficult to get datetime objects into xarray objects -- normally they are converted into datetime64.

shoyer · 2017-03-05T02:01:48Z

xarray/plot/plot.py

+    # Warn then.
+    for t in numpy_types:
+        if np.issubdtype(np.generic, t):
+            warnings.warn('{} is treated as numpy.generic.'.format(t),


This is an internal, private function. Rather than warning let's issue a ValueError to ensure it doesn't happen by accident. It would also be safe to just use an assert here, since again it should never happen in user code.

Thanks for the review! I've changed it using assert.

shoyer · 2017-03-05T02:02:45Z

xarray/tests/test_plot.py

@@ -1208,3 +1213,25 @@ def test_default_labels(self):
        # Top row should be labeled
        for label, ax in zip(self.darray.coords['col'].values, g.axes[0, :]):
            self.assertTrue(substring_in_axes(label, ax))
+
+
+@requires_netCDF4


This test shouldn't be netCDF4 specific anymore.

That's right. I've changed it to use datetime and to not require netCDF4.

shoyer · 2017-03-05T02:03:16Z

xarray/tests/test_plot.py

+
+        darray = DataArray(data, dims=['time'])
+        darray.coords['time'] = \
+                np.array([nc4datetime(2017, m, 1) for m in month])


Again, just use a normal datetime instance now.

shoyer · 2017-03-05T02:04:04Z

xarray/tests/test_plot.py

+        data = np.sin(2 * np.pi * month / 12.0)
+
+        darray = DataArray(data, dims=['time'])
+        darray.coords['time'] = \


style: avoid \ in favor of implicit line continuation with parentheses: https://www.python.org/dev/peps/pep-0008/#maximum-line-length

- Use datetime instead of netCDF4.datetime. - Do not warn but assert in private function.

willirath · 2017-03-06T13:37:33Z

Yes, I know that xarray usually doesn't use datetime. For climate data past 2262 with a standard calendar, however, the dtypes of the time axis are falling back to datetime / netCDF4.datetime.

My last commit reflects what I'd consider ready to be pulled. Let me know what you think.

shoyer

Oops -- meant to post this yesterday but didn't hit submit on the review. Two minor suggestions, otherwise looks good to me.

shoyer · 2017-03-06T00:52:50Z

xarray/plot/plot.py

    """
-    plottypes = [np.floating, np.integer, np.timedelta64, np.datetime64]
+    return all(any(isinstance(el, t) for t in types)
+            for el in np.ravel(x))


style: the line continuation here should start after the open parenthesis after all

Done. (Line was 79 chars long anyway.)

shoyer · 2017-03-06T00:54:51Z

xarray/plot/plot.py

+
+    for x in args:
+        if not (_valid_numpy_subdtype(np.array(x), numpy_types)
+                or _valid_other_type(np.array(x), other_types)):


Probably would be a good idea to ensure that x has object dtype (either here or in _valid_other_type) to avoid potential expensive loops, e.g., if dtype=complex but x is a very large.

I've added a check to _valid_other_type which ensures np.issubdtype(np.generic, x.dtype) before looping over x. (See feb6afb)

shoyer · 2017-03-09T21:43:54Z

Thanks!

Allow for plotting dummy netCDF4.datetime objects.

32e2235

shoyer reviewed Feb 12, 2017

View reviewed changes

willirath mentioned this pull request Feb 12, 2017

xarray.open_mfdataset returns inconsistent times #1263

Closed

willirath added 3 commits February 13, 2017 11:06

Add decorator to skip if netCDF4 is missing.

a85b5c6

Test line plot with netCDF4.datetime coordinate.

e31aaab

Rewrite checks for plottable data.

6f5e30f

Allow for numeric and date-like numpy data types and for datetime objects.

shoyer reviewed Mar 5, 2017

View reviewed changes

Address review by @shoyer

35b9cd6

- Use datetime instead of netCDF4.datetime. - Do not warn but assert in private function.

shoyer reviewed Mar 6, 2017

View reviewed changes

willirath added 2 commits March 6, 2017 21:33

Line continuation after parenthesis.

0e39637

Check for generic dtype to avoid expensive loop

feb6afb

shoyer merged commit 83d2c2d into pydata:master Mar 9, 2017

Uh oh!

Allow for plotting dummy netCDF4.datetime objects. #1261

Allow for plotting dummy netCDF4.datetime objects. #1261

Uh oh!

Conversation

willirath commented Feb 11, 2017

Uh oh!

shoyer commented Feb 11, 2017

Uh oh!

willirath commented Feb 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willirath commented Feb 12, 2017

Uh oh!

willirath commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willirath commented Feb 13, 2017

Uh oh!

willirath commented Feb 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willirath commented Feb 13, 2017

Uh oh!

shoyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willirath commented Mar 6, 2017

Uh oh!

shoyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willirath Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shoyer commented Mar 9, 2017

Uh oh!

Uh oh!

willirath commented Feb 12, 2017 •

edited

Loading

willirath commented Feb 13, 2017 •

edited

Loading

willirath Mar 6, 2017 •

edited

Loading