issue_comments

Thanks @fmaussion!

Oh, sorry. LGTM.

OK, this looks good to me. I'm just waiting on @mwaskom before merging.

Right, I forgot to fetch from upstream, now it should be all clean.

This PR was a good experience for me! I started using xray a few weeks ago for teaching and it's really awesome to see how students with no prior experience with python are able to get the logic of working with reanalysis data so quickly...

@fmaussion try pulling the latest master and rebasing this branch? If that doesn't work for you, I can apply this change manually.

Ups. I did a "fixup" which seems to have broken github. I'm sorry for the mess... What should I do now? I tried it locally and my branch (which consists one one single commit now) can be fast-merged...

This looks good to be -- the tests seem to nail this down pretty well, even though the logic has indeed become more convoluted. @mwaskom any concerns from your side?

Please do do a rebase/squash (most importantly to eliminate any commits that failed CI) and also please add a shirt change note to a "breaking changes" section of the what's new docs. This will probably not make it in until the 0.7 release (which probably will be our next release), but just go ahead and put it in the 0.6.2 notes for now and we'll reorganize later.

I've incorporated both comments and added the documentation. Let me know if it still needs revision. Should I do a rebase?

I forgot to mention: I tried to reach a compromise between @jhamman and @shoyer in the following:

python # Setting non-centered vmin, vmax prevents a divergent cmap if (vmin is not None) and (vmax is not None): if not np.isclose(vmax - center, center - vmin): divergent = False

I don't know if np.isclose() is good enough for the numerical issues @shoyer mentioned.

Hi @jhamman,

1. If you set anything, it should not be overwritten. This includes the colormap and vmin/vmax.
2. If you want very specific/consistent behavior, you'll need to explicitly provide those arguments.

This is the actual purpose of my PR.

The only safe times to assume a diverging colormap is when vmin = -vmax or when center is not None. If you want a diverging colormap for other situations, you need to specify cmap in the plot call.

I tend to agree with that but this would break backwards compatibility. Previous behavior was to decide that if vmin < 0, we have a diverging cmap. This logic remains in my PR, but I can change it if you gurus decide otherwise.

Some of the specs added a bit of boilerplate code, especially that one:

if only one of vmin/max is provided and the colormap is diverging, then the other should be determined by reflecting the limit around the center

but my changes remain quite small and seem to have preserved backwards compatibility. I tried to cover all the use cases in my test function (test_divergentcontrol).

Critics and/or additional use cases welcome.

1. The only safe times to assume a diverging colormap is when vmin = -vmax or when center is not None. If you want a diverging colormap for other situations, you need to specify cmap in the plot call.

I actually agree with @mwaskom here. I would not make inferring a diverging colormap dependent on vmin = -vmax (assuming center=0). It's useful to be able to control the bounds of a diverging colormap by only setting one of vmin and vmax, and comparing vmin - center == center - vmax seems prone to numerical precision issues.

I do agree with principles 1 and 2, though.

The only safe times to assume a diverging colormap is when vmin = -vmax or when center is not None. If you want a diverging colormap for other situations, you need to specify cmap in the plot call.

That's fine for xray to have that opinion. Unfortunately, I think it's going to mean the behavior is going to diverge (heh) from what happens in seaborn.

I'll just chime in here quickly and layout how I think this should go - 1. If you set anything, it should not be overwritten. This includes the colormap and vmin/vmax. 2. If you want very specific/consistent behavior, you'll need to explicitly provide those arguments. 3. The only safe times to assume a diverging colormap is when vmin = -vmax or when center is not None. If you want a diverging colormap for other situations, you need to specify cmap in the plot call.

All in all, I think what I'm getting at is we may want to tighten up on when we switch from the default cmap to the diverging cmap.

OK, my last push will probably brake some tests. You can have a look at my code and test-cases to see if you agree with the behaviour. I have to go home now, but I can work on this again tomorrow morning (CET)

Including this last change (if both vmin and vmax are provided, the logic is that of a non-divergent cmap) brakes several tests.

That's the behavior I would want seaborn to have, but I guess @shoyer can weigh in too since it affects xray as well.

Ok, I have come up with something not too complicated. But before I push the changes, do we agree that when both vmin and vmax are provided, the logic is that of a non-divergent cmap?

OK, so the test case would look like:

python data = np.linspace(0, 1, num=100) - 0.1 cmap_params = _determine_cmap_params(data, vmin=-0.1) self.assertEqual(cmap_params['vmin'], -0.1) self.assertEqual(cmap_params['vmax'], 0.1) self.assertEqual(cmap_params['cmap'], "RdBu_r")

This of course requires a bit more logic than the simple cases I handled in the first PR. I'll do my best.

I think that if nothing else than preserving some backwards compatibility, you should take the route that shoyer suggested and assume that data spanning 0 with center not explicitly set and at most one of the limits specified should get a diverging palette.

@mwaskom correct. I forgot to read @shoyer 's comments about this. He said:

If only one of vmin/max is provided and the colormap is diverging, then the other should be determined by reflecting the limit around the center -- not by taking the largest distance from the center. If this results in data falling beyond the color limits, then so be it (in xray, this will be displayed with the extend argument on the colorbar). For example, suppose my data contains values between -10 and 100. If I plot it with vmin=-10 and vmax=None, then (1) we should detect diverging data with center=0 (unless center=False) and (2) we should infer vmax=10.

I agree with this, but only if center is explicitly provided. I'll try to implement this.

It seems like this will use sequential logic if you set center to a value and only one of vmin or vmax? I would think in that case it looks like the user is asking for a diverging palette.