issue_comments

Thanks @Zac-HD !

@shoyer makes some good points about things that should be fixed upstream in Matplotlib - namely normalization of RGB images, but I'm also going to move my color fixes (clipping instead of modulo) upstream instead. Timeline:

1. I edit this PR, it gets merged, Xarray 10.1 (ETA soonish?) can display RGB images just like Matplotlib
2. I finish my upstream PR, Matplotlib 2.2 (ETA later this month (!)) supports normalization of RGB images and fixes the modulo/clip bug
3. Once the matplotlib API is merged, I make a new PR against Xarray to support the same interface (including on older matplotlib versions)

Should be fixed by https://github.com/pydata/xarray/pull/1814 which I just merged

Looks like TestDatetimePlot.test_datetime_line_plot is failing, but I'm pretty sure that's nothing to do with me (and it passed twice on Travis!).

@Zac-HD Sounds good. At this point I don't think there are any blockers left for the 0.10.1 release (I wanted to get the pydap fix in, which we recently merged).

Okay... I've now seen three Travis runs. In every one there's been a pass, a fail, and three errors... but different jobs each time. At this point I'm ready to give up and wait for the Travis team to fix it 😕

The code is ready to go though! 🎉

Just restarted them, let's see how it goes

More review fixes 😄

I'm pretty sure the build failure is just Travis being flaky - it passes on my machine. Could someone restart it?

OK, @shoyer & @fmaussion - I think I'm done again!

The only thing I haven't done is change the handling of robust=True, as anything beyond "scale and clip 2nd-to-98th percentiles to [0, 1]" seems fraught with complications. If this is not what the user intended, it's at least the obvious default behaviour and should be simple to understand.

Hopefully this covers the substantial changes; but let me know if there's anything else as I'd really like this merged before the 0.10.1 release - and ideally released in time for demos at my summer school in January 😄

Thanks for the review @shoyer! I'll do what I can in the next few days, but that might not be much at all before I get back from a no-internet camping trip around Jan 8th. Items:

• [x] add validation and tests for rgb argument
• [x] don't mutate the input data! (I think it doesn't, but obviously this needs to be checked)
• [x] simplify order heuristic, to "use last dimension of size 3 or 4 as rgb; warn if there are multiple possible"
• [x] update rasterio example in gallery

I'll add another todo (can be done in a separate PR if you don't have time ;-): modify the gallery example to plot the RGB image instead of the BW with our rasterio example here.

Done - now with optional argument and nicer commit history, Anything else?

Optional argument coming up then 😄

Anything else I need to do?

I should clarify: I don't think we should require the explicit name for the RGB dimension, but I think we should have an argument around so that's an option if desired. This is nice because it gives better error messages when things go wrong and makes it easier to write self-documented code. This would be consistent with the rest of our visualization functions, which provide an option for being fully explicit but don't require it.

I like this. My main concern is that I'd like a fully explicit way to opt into this behavior. Perhaps rgb='band' and/or rgba='band'?

I have a pretty strong preference for RGB images 'just working' - requiring a new argument would mean imshow has a different signature to all other plot methods without adding any information. If it's an optional argument, it barely does anything - in the rare case where the heuristic fails, you can supply x and y dims instead.

In many ways showing an RGB image is a special case, but this is the least-special-case I could make work - and it's consistent with the obvious meaning and upstream behavior of imshow.

I like this. My main concern is that I'd like a fully explicit way to opt into this behavior. Perhaps rgb='band' and/or rgba='band'?