Fix contour plot colorscale domain

[lvlte]

Fixes #6623

Basically, it allows to make the colorscale domain fit a fixed range [min, max], whether by setting zmin/zmax or coloraxis cmin/cmax, which was not possible before except for the "heatmap" contour coloring method.

This is still possible to obtain the old behavior, ie. when the contours are defined to focus around median values (contour start/end far from min/max in z), then narrowing zmin/zmax (cmin/cmax) accordingly is fine.

Before vs After.

[lvlte]

NB. Regarding fb7e181,

See this test case, which otherwise would complain about fill level attributes changing between transition/redraw, having style: '' vs style: 'stroke: none;', I think this was not happening before the fix because none of the fill levels rect to draw were missing (in the enter selection) after transition.

[lvlte]

NB. Regarding the update of some mocks ae34826

In the airfoil and h-colorbar_airfoil mock, there is a contourcarpet plot which displays (without the fix) a colorbar ranging from -1 to +1. The min value of z is -6.04, and the config has zmin: -8, so the colorbar should start at -8 instead of -1. With the fix, zmin is taken into account so the colorbar starts form -8, setting zmin: -1 it starts at -1. Since the intent for these plots is to have the colorscale range equals to [-1, +1], I updated the mock config accordingly (and ironically for these plots the baseline is exactly the same at the end).

In contour_edge_cases, set zmin and zmax for some traces to keep focus on precise contours levels that are far from zmin/zmax and let the colorscale "open" (ie. as without the fix) above/below the central values, in this case setting zmin: <contours.start> and zmin: <contours.end> is fine. Which I also applied in the mock config.
Same for contour_log, contour_valid_ses, histogram2dcontour_bingroup-coloraxis

For those plots with computed tickvals/ticktext (not explicitly set in config) and for which the colorscale has changed, we may obtain different tickvals/ticktext (expected). The tick prefix/suffix if any are properly applied according to those calculated ticks.

[archmoj]

Could you please explain this change?

[lvlte]

Well, it's a bit tricky to explain.. The first thing to note is that when calling Plotly.newPlot(), the colorbar is drawn twice, ie. the code above runs twice (which might be a bug (?) as the first draw seems correct, but I didn't dig much into that). When calling Plotly.react() though, it runs once.

Now, we have (before this commit) :

var fills = g.select('.' + cn.cbfills)
    .selectAll('rect.' + cn.cbfill)
    .attr('style', '')        // -> existing <rect> have empty `style` attribute
    .data(fillLevels);
fills.enter().append('rect')
    .classed(cn.cbfill, true)
    .style('stroke', 'none'); // -> missing <rect> are added with `style='stroke: none;'`
    fills.exit().remove();

So, if I'm not wrong :

• After calling Plotly.newPlot(), because the code runs twice, there is no missing <rect> on the second pass obviously, and in the end all of them have an empty style attribute (for contour colorbar, each element get colored via the fill attribute)
• After calling Plotly.react() however, because the code runs once, those <rect> that were newly added, if any, will have in the end style='stroke: none;'

This discrepancy causes this test case to fail, because some attributes are changing between transition/redraw. So this commit is just to fix that.

Now the question is : how the heck this test case hasn't ever failed until this PR ?

I think this could have to do with the double call and possible race conditions.. Hopefully I've been able to reproduce the failure without the colorbar fix, by using a mock with more data than those used previously by the test (longer to draw, hence the suspicion about race conditions), also fixed the promise chaining. I'm going to commit that so you can double check all of this.

[archmoj]

Very interesting PR. 🎖️
To make it clear the impacts of this PR on the baselines, I suggest you revert the changes to the mocks.
We could reapply them after this PR.
Thank you!

[alexcjohnson]

@lvlte very interesting, thanks for this PR. In most cases I think this exactly the right change - in the fully-auto case now we're representing the full data range in the colorbar, which is the main thing. Even when it results in a substantial change in the colorbar, like the airfoil and h-colorbar_airfoil mocks, for some reason these specify zmin and zmax and currently we just ignore those, but this PR makes us honor those. That said, it looks like we'll only honor them if they're outside the range of the contours (+/- half a contour on either end) - shouldn't we also allow users to specify a narrower range and honor that in the colorbar?

The other case I think might not be what we want is when you specify contours: {start, end} but do NOT specify zmin or zmax. That's the case in the contour_edge_cases mock for example, all but the top two subplots, where including the implicit zmin and zmax from the data range collapses your specified contours into invisibility in the middle of the colorbar. I don't think it's reasonable to ask the user to also specify zmin and zmax in that case, that feels duplicative. Can we add logic like "if your input trace specifies contour start and end, use start-size/2 and end+size/2 as the default for zmin and zmax instead of the data min/max"?

[Coding-with-Adam]

hi @lvlte Thank you for taking the time to create this helpful PR.

We've been talking about a new project we'd like to pursue within Plotly.js, and your name came up 🤗

Would you be willing to share your email address with me so I can tell you more about it. Feel free to email me directly at adam@plot.ly

Thank you,

[lvlte]

@alexcjohnson

it looks like we'll only honor them if they're outside the range of the contours (+/- half a contour on either end) - shouldn't we also allow users to specify a narrower range and honor that in the colorbar?

For now, if we use autocontour and specify a narrower z range than that of the actual z data, since the contours start/end are automatically picked within that range, then we are somehow able to narrow the colorbar as needed.

However, if a user specifies both contours start/end and zmin/zmax, and if zmin > start and/or zmax < end, we can honor that in the colorbar but we may obtain a plot with regions where z < contours.start or z > contours.end are filled with colors not included in the colorbar, or filled with the same color than the first/last contour level regions (I don't get how/why for now, I'm gonna commit that change anyway so that you can see what I mean). Shouldn't we rather prevent users from setting contour start/end that are outside the real range of the z min/max data ? Otherwise, how to properly determine what fill color should be applied for those regions where z is out of the colorscale (I'd say no fill style at all, only the contour lines) ?

when you specify contours: {start, end} but do NOT specify zmin or zmax [...] can we add logic like "if your input trace specifies contour start and end, use start-size/2 and end+size/2 as the default for zmin and zmax instead of the data min/max"?

Yes this makes total sense, it's not reasonable to oblige the user to specify zmin and zmax in that case.

[lvlte]

@Coding-with-Adam Hi, thanks for reaching out ! You should have an email from me in your inbox.

[archmoj]

Suggested change

	if(typeof trace._input.zmin === 'number' && typeof trace._input.zmax === 'number') {
	if(trace._input && (typeof trace._input.zmin === 'number') && (typeof trace._input.zmax === 'number')) {

[lvlte]

Hey, I was doing some testing and just realized there are still issues with my code (if user explicitly sets larger z range with smaller contour size), so it needs more work, I will fix that tomorrow I think.

[archmoj]

@lvlte The PR looks very good. However I noticed something strange is going on with histogram2dcontour_bingroup-coloraxis mock. It looks like the traces do not pick up the correct colors from the coloraxis. This is also happening on the master branch. Am I missing something? If no we could report a bug and fix it in another PR.

[lvlte]

@archmoj yes it looks like a bug, if I'm not wrong we should have the same colors for the contour fills in histogram2dcontour_bingroup-coloraxis as for the bins in histogram2d_bingroup-coloraxis.

In fact, it seems like the colorbar is built using contours from the first trace, the zmin/zmax extremes seem correct (extremes from all traces), but each trace builds its own colorMap from its own (auto)contour attributes (start, end, size), and since these contour attributes are computed independently for each trace, their colorscale domain is independent from the "main" colorscale (first trace).

[archmoj]

💃