issue_comments

Thanks for this @jbusecke! Really excited to have this feature in Xarray now.

thanks @jbusecke

Can you also add line somewhere at the top of plotting.rst saying that cftime plotting is supported once nc-time-axis is installed?

@jbusecke Can you also add nc_time_axis to print_versions() if it is installed?

I think it would make sense to follow this example, but use cftime.datetime objects instead. You might want to add a test for a 2D plot just to be sure. https://github.com/pydata/xarray/blob/2e99c7dde32e396dd2c5fc6261a2d06217621e10/xarray/tests/test_plot.py#L1797-L1813 Note you'll also need to add nc-time-axis to some CI environments so that things run in some Travis/AppVeyor builds, probably best in: - ci/requirements-py37.yml - ci/requirements-py37-windows.yml

and add some decorators to skip the tests if the needed packages (cftime and nc-time-axis) are not installed. @requires_cftime already exists, but I think you'll have to write your own @requires_nc_time_axis decorator, which you can do, e.g., here: https://github.com/pydata/xarray/blob/2e99c7dde32e396dd2c5fc6261a2d06217621e10/xarray/tests/init.py#L65 Maybe by leaving nc-time-axis out of the py36 test environment (which has cftime) you can use it to test the error message if nc-time-axis is not installed?

@jbusecke SciTools/nc-time-axis#42 has been merged, and a new release has been made (it's already available on conda-forge)! It would be great if you could update this PR to use this latest version of nc-time-axis -- you should no longer need to convert dates to CalendarDateTime objects.

Thanks again @lbdreyer for your help.

I agree @dcherian; I just pinged the PR again, but if there is no activity there by this time next week, I think we should probably move forward here.

Looks like upstream hasn't moved. Maybe @jhamman and @spencerkclark can re-ping for a review there?

I am +0.5 on moving forward with an xarray workaround. It seems easy to remove once upstream makes all the required changes.

I pinged the met-office folks about your PR. Hopefully that get's merged.

I appreciate it @jhamman; we'll see what happens there.

Oh shoot, I now remember seeing this. If this will be implemented soon I guess the PR can be discarded.

Or this PR could be amended :). We'd still need to make some changes to xarray along the lines of what you've started on here for the optional import of nc-time-axis, addition of cftime.datetime as a plottable type, and updates to the error messages.

Any chance you would have a quick solution for the pcolormesh plot error (second example in the PR)

CalendarDateTime objects are limited in the operations they support, e.g. >= is not supported, which is used in _infer_interval_breaks, which by default is called in xarray's pcolormesh. So this is one place in xarray where being able to use true cftime.datetime objects would really help. Otherwise you'd need to either wait to convert to CalendarDateTime until just before you passed data to a matplotlib function in xarray, or hack _infer_interval_breaks to make it compatible with input arrays of CalendarDateTime objects by converting to them cftime.datetime and back.

If you're in pinch I think things would work here if you passed infer_intervals=False as an argument to plot: python da2.plot(infer_intervals=False) though in general infer_intervals is used for a reason (see https://github.com/pydata/xarray/issues/781#issuecomment-191546524).

@spencerkclark - I pinged the met-office folks about your PR. Hopefully that get's merged.

Note I have a PR open in nc-time-axis, which enables plotting of cftime.datetime objects directly (without having to convert to CalendarDateTime objects). This would make this easier in xarray and elsewhere.

That said, I'm not sure if/when it will be merged, so it probably makes sense to go forward with this approach for now.