issue_comments

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...

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...

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.

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.

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.

@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.