issue_comments

cool! Thanks

Thanks @nbren12 !

I just rebased onto master.

I couldn't find a .stickler.yml file in this repository. I can make one for you, or you can create one by following the documentation.

no problem. I have always preferred putting operators on new lines though. Didn't realize that was against pep8. oh well 🤷‍♂️ .

Sorry, our new flake8 tool can be quite strict ;)

Sorry @nbren12 there is still a pep8 error somewhere, and you can freely make it an error as @shoyer suggests.

Hopefully, the commit I just pushed fixes this. I had some global flake8 settings that were messing with my local linting.

Note that currently the test for monotonic coords is for pcolormesh only, while it could be for all 2d plots as well.

I don't think I was having any issues with contour or contourf when I first opened the issue, but it probably does break imshow.

If we start to check input for sanity we might as well raise an error when coordinates are not regularly spaced in the imshow case.

Maybe we could leave this to a later PR.

Sorry @nbren12 there is still a pep8 error somewhere, and you can freely make it an error as @shoyer suggests.

Note that currently the test for monotonic coords is for pcolormesh only, while it could be for all 2d plots as well. What I like with the current implementation is that the check happens in _infer_interval_breaks, which is only called when certain conditions are met (1D coords, or 2D coords without transform kwarg). If this check lives in the common 2d function the same conditions should be applied.

If we start to check input for sanity we might as well raise an error when coordinates are not regularly spaced in the imshow case, although I can imagine this might be more difficult to check.

Thoughts?

Yah. I knew my solution before was probably too cute.

On Tue, Feb 6, 2018 at 11:14 AM, Fabien Maussion notifications@github.com wrote:

Thanks! This looks good.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/1885#issuecomment-363532796, or mute the thread https://github.com/notifications/unsubscribe-auth/ABUokvVDiWCnX_JPa5TsDtW8JtQxdTDEks5tSKSZgaJpZM4R4EWU .

Thanks! This looks good.

There. I think this code should work for all dtypes and 2D coords.

Which, I guess, illustrates our point quite well: while it's easy for users to find out when their data is messy, it's much harder to find a robust and logical way to handle all cases for a general library like xarray. I guess it's the reason why matplotlib also doesn't warn or error when unordered data is given to pcolormesh.

Don't get me wrong: I think it would be valuable to be able to provide a better "service" than matplotlib. Probably we should even error earlier, at the 2d plot level (and not only at the pcolormesh level like it is now). It's just going to be a bit hard because of all the special cases (dtypes, 2d coords, and map projections moslty).

Ordering "per default" might be the most straightforward way, but: - what about performance issues? - I don't like the idea of silently doing things with user data

I'll let @shoyer decide on what's best here. I am slightly in favor of doing nothing and giving the responsibility to the user.

One reason is that it's not obvious if they would like increasing or decreasing coordinates.

For me at least, plt.pcolormesh automatically displays in increasing order even if one of the input arrays is sorted in descending order. This happens all the time with meteorological data available in pressure coordinates (pressure goes down with height). I usually have to manually call plt.gca().invert_yaxis() to flip the y axis. Does xarray.plot's behave differently?

Is there any chance people would want to make heatmaps involving categorical variables though?

Certainly, but the currently version of 2D plots in xarray doesn't work for these!

If they want to make plots involving categorical variables, I would currently recommend using seaborn.heatmap (we can mention it in the error message, too).

If we do decide to raise an error, why not go one step further and just sort the coordinates automatically?

One reason is that it's not obvious if they would like increasing or decreasing coordinates.

@shoyer That would work with me. Is there any chance people would want to make heatmaps involving categorical variables though?

If we do decide to raise an error, why not go one step further and just sort the coordinates automatically?

On Sun, Feb 4, 2018 at 4:00 PM, Stephan Hoyer notifications@github.com wrote:

@shoyer commented on this pull request.

In xarray/plot/plot.py https://github.com/pydata/xarray/pull/1885#discussion_r165863769:

@@ -750,6 +767,13 @@ def _infer_interval_breaks(coord, axis=0): [ 2.5, 3.5, 4.5]]) """ coord = np.asarray(coord) + + if not _is_monotonic(coord, axis=axis): + warnings.warn("The input coordinate is not sorted in increasing order "

Rather an a warning, why not make this an error? I don't see any use-cases for 2d plots with non-monotonic coordinates. With the current version of xarray, these plots always end up wrong in some way, either by not plotting everything or with bad axis labels.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/1885#pullrequestreview-93863697, or mute the thread https://github.com/notifications/unsubscribe-auth/ABUokuMwNBc8JRgafrF7HNmGfgzxi64Qks5tRkSbgaJpZM4R4EWU .

Ok. I think everything is ready now.

Thanks for your help @fmaussion. Hopefully the commit I just pushed fixes the failing tests. I will work on adding a test and fixing the formatting.

Thanks a lot @nbren12 ! Indeed _infer_interval_breaks is probably the right place to add this warning.

A few comments: - the tests are now failing, likely because of a type issue with time coordinates - coordinates don't have to be ascending for pcolormesh to work (see example below). Monotonically ascending and descending coordinates should work. - we need a test for this new behavior (asserting warnings) - we follow the pep8 conventions, and I think your lines are longer than the 79 character rule

python import numpy as np import xarray as xr x = np.arange(5)[::-1] y = np.arange(6)[::-1] x = xr.DataArray(x, dims=['x'], coords={'x': x}) y = xr.DataArray(y, dims=['y'], coords={'y': y}) z = x + y z.plot();