issue_comments

Failed test is cfgrib test.

Thanks @maahn

Hello @maahn! Thanks for updating the PR.

• There are no PEP8 issues in the file xarray/plot/__init__.py !

• In the file xarray/plot/plot.py, following are the PEP8 issues :

  Line 1062:13: W504 line break after binary operator Line 1063:14: W504 line break after binary operator Line 1072:13: W504 line break after binary operator

• There are no PEP8 issues in the file xarray/plot/utils.py !

• There are no PEP8 issues in the file xarray/tests/test_plot.py !

Comment last updated on October 23, 2018 at 06:44 Hours UTC

great, thanks for taking care of that!

Really sorry for the delay, @maahn.

I've merged master, refactored out the utility functions to utils.py, fixed the tests locally and added a whats-new entry. I'll merge once this round of tests pass.

Sorry, for the delay, I finally merged upstream. Looks like the failed builds are unrelated to my changes, so it should be ready for merging?

Thanks for the review.

Good idea, it turned out the step function was quite easy to implement by using https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/axes/_axes.py#L1735 So now, 1D data defaults to the standard line plot, but you can use plot.step() instead. I guess changing the default plot to something more sophisticated would make the simple logic in plot() to determine the default quite complex?

See https://gist.github.com/maahn/91da0a8d299ef6567827749cbe2f1913

Rather than using interval_step_plot=True, what about making an entirely new plot method, plot.step()? That would feel a little cleaner to me.

I'm not sure that a step plot would be preferred by most users for a plot over an axis on which groupby_bins was applied. Personally, I would probably prefer a line + scatter plot (e.g., marker='s'). Maybe @rabernat (groupby_bins author) has opinions here?

Ok, I added the interval_step_plot kwarg (default True) to the 1D line plot to make a step plot with the real (i.e. not interpolated) boundaries. After that I had the feeling that it's inconsistent if line uses the real boundaries but pcolormesh doesn't. So I also patched that, but I had to disable infer_intervals for these cases. See also: https://gist.github.com/maahn/91da0a8d299ef6567827749cbe2f1913/530ed7bf77c9c3257cab46c3894c1412085a217a

Let me know what you think and I will also add some documentation.

A couple of other thoughts:

1. For 2D plots that already show regions (imshow/pcolormesh), I'm not sure it makes sense to update the label to include the word "center". This plot around shows intervals pretty clearly:
2. This needs tests, at least something to make sure that plotting with an Intervals on an axis does not crash in xarray/tests/test_plot.py

Thanks, for the comment, but I wouldn't use center labels when using groupby_bins by default, that could be misleading in case of non-uniform (e.g. exponential) bin spacing. I guess that's acceptable for a plot, but not for a DataArray, because the information about the boundaries would be lost. And from a user perspective, I would find it a bit confusing if an additional coordinate would show up when using groupby_bins.

This seems like a good idea.

groupby_bins has a labels option, which can override the default labels generated by pandas. That's what is done in the (multidimensional groupby example](http://xarray.pydata.org/en/stable/examples/multidimensional-coords.html#multidimensional-groupby).

I wonder if it's worth just making this behavior the default, or generating an additional coordinate for the bin centers automatically.