Yes, looks good. Merge away!

@shoyer - have a good time.

@clarkfitzg - I think I've addressed all your comments, including returning a dict from _determine_cmap_params. If there is nothing else, I'll merge this tomorrow.

@jhamman you should merge this once you and @clarkfitzg are happy. I'm about go offline for 6 days (backpacking in the Olympics).

Nice work @jhamman @shoyer. Tests look pretty thorough and I like all the helper functions.

@clarkfitzg any thoughts here?

These tests look great -- thanks!

@shoyer - I added a bunch of tests this morning and checked off the list of documentation and error handling items. Take another look and let me know if you think we need anymore tests or changes to the docs.

This looks good. Thanks for doing that.

Are there any cases where you would want to set vmin/vmax to something other than the first and last levels?

Yes, but they are rare and you could pass in a custom norm in those cases.

This assuredly still needs more tests -- mostly to verify that every plot types ends up with consistent levels.

I'll think a bit more about how to test these aspects.

There are three things left to do here (this has gotten a big bigger than I was expecting): - [x] Decide what to do with seaborn's handling of jet - [x] Fill in tests - [x] Add an example in the plot docs

Decided to try reorganizing things and got sucked in, so I ended up making a PR to your fork with some tweaks: https://github.com/jhamman/xray/pull/1

This assuredly still needs more tests -- mostly to verify that every plot types ends up with consistent levels.

Here's my demo notebook: https://gist.github.com/shoyer/e6fafbc672021600d9f3

and a teaser:

The main difference in functionality is in my PR is that vmin and vmax cannot be set independently of levels, if levels is provided as a list. That seemed a little too flexible, and this way we can get a consistent result across all four plot types. Are there any cases where you would want to set vmin/vmax to something other than the first and last levels?

Also, I entirely replaced the levels calculation in contour/contourf with our calculated levels instead of merely trying to replicate it. That ensures things are guaranteed to be exactly identical across all plot types.

@shoyer - This last commit (119c1c8) should have addressed the issues you brought up. I updated my examples a bit to show how things are working:

``` python %matplotlib inline import matplotlib.pyplot as plt import numpy as np import xray

sample DataArray

x = np.arange(start=0, stop=10, step=2) y = np.arange(start=9, stop=-7, step=-3) xy = np.dstack(np.meshgrid(x, y)) distance = np.linalg.norm(xy, axis=2) distance = xray.DataArray(distance, list(zip(('y', 'x'), (y, x))))

Sample plots

fig, axes = plt.subplots(3, 2, figsize=(8, 8), sharex=True, sharey=True) distance.plot(ax=axes[0, 0]) axes[0, 0].set_title('Default') distance.plot(vmin=0, vmax=12, levels=6, ax=axes[1, 0]) axes[1, 0].set_title('Setting with integer levels') distance.plot(levels=[1, 2, 4, 5, 7, 9], extend='max', cmap='Spectral', ax=axes[0, 1]) axes[0, 1].set_title('Setting with list of levels') flatui = ["#9b59b6", "#3498db", "#95a5a6", "#e74c3c", "#34495e", "#2ecc71"] distance.plot(levels=[1, 2, 4, 5, 7], cmap=flatui, extend='both', ax=axes[1, 1]) axes[1, 1].set_title('Using custom list of colors') distance.plot.contourf(ax=axes[2, 0]) axes[2, 0].set_title('Default contourf')

distance.plot.contourf(levels=4, ax=axes[2, 1]) axes[2, 1].set_title('contourf w/ int levels')

plt.tight_layout() ```

If the user sets the extend, vmax, or vmin arguments, those are not overridden. This is shown in the top right plot where I set extend='max' even though the min of the data is less than the min of levels. This results in the data less than the min of levels not being show. For the same reason, we also don't use the MaxNLocator if vmin or vmax was provided.

Options for when levels is provided and extend == None:

I would opt for your solution 2. I think this basically means using levels[0] and levels[-1] if levels are provided explicitly instead of vmin/vmax (which are being ignored anyways).

The logic for inferring the extend keyword argument needs to be updated to handle levels:

How do you want to handle this? Options for when levels is provided and extend == None: 1. Don't extend. This will result in data outside the range of levels being masked out, 2. Extend if the min/max of the data exceed the range of levels, or 3. Require extend to be specified when levels is provided.

I will add the MaxNLocator ticker for integer levels. I agree this is the way to go.

I think this logic will also help address your 4th point:

...contourf currently crashes if you supply levels as an integer. This should be fixed, probably by converting levels into a 1d array / list if they are supplied as an integer before passing it into contourf. This would actually be more broadly useful, because currently there is no way to supply a non-default integer number of levels with contourf -- matplotlb appears to only expose the N argument when it is used positionally.

Looking at the matplotlib source code, here's how they figure out levels in contour/contourf: https://github.com/matplotlib/matplotlib/blob/714d18788320325d0bff75184f62d472f67ceb91/lib/matplotlib/contour.py#L1143

It looks like they use MaxNLocator to do this: http://matplotlib.org/api/ticker_api.html#matplotlib.ticker.MaxNLocator

It should be pretty straightforward to use that here.

Some feedback: 1. matplotlib seems to have nice heuristics for picking level splits with contour plots. I wonder if it would be feasible to copy that logic here when setting our own discrete levels? Compare: ds.t2m[3].plot(levels=6) vs ds.t2m[3].plot.contourf() 2. The logic for inferring the extend keyword argument needs to be updated to handle levels: ds.t2m[3].plot(robust=True, levels=[200, 250, 260, 270, 280, 290, 350])

3.imshow, pcolormesh and contourf seem to deal with an explicit list of levels the exact same way. That's a relief. The plots look exactly the same. 4. As you noted, contourf currently crashes if you supply levels as an integer. This should be fixed, probably by converting levels into a 1d array / list if they are supplied as an integer before passing it into contourf. This would actually be more broadly useful, because currently there is no way to supply a non-default integer number of levels with contourf -- matplotlb appears to only expose the N argument when it is used positionally.

These last two commits have resolved the two issues I brought up above.