Interpolate contours in linear space, not data space

[andrew-matteson]

Fixes #5899

Contours are determined by interpolation. When the x or y axis is non-linear, the interpolation should be non-linear.

This PR changes calculates a linearized dx and dy, then uses then converts back into pixel space. I've added a test to the dashboard of contour plots of 4 sparsely sampled "cones" that should all have the same fill colors/shapes if interpolation is handled correctly.

[archmoj]

Thanks very much for the PR.
Now that #5986 merged,
would you please fetch upstream/master then merge it into this branch so that we get all green tests?

[archmoj]

@alexcjohnson I think the way contours are working on master is actually correct.
Here is a codepen.
If so this PR should add a new feature?

[alexcjohnson]

"correct" is a bit open to interpretation when it comes to interpolation, but I think what @andrew-matteson has implemented here is more useful. If you've displayed your data on a log axis, the implication is that variations tend to be smoother on a log scale, hence that's the space in which it makes sense to interpolate it.

For reference here's what the new mock would look like with the existing interpolation:

Pretty obviously worse than the version added in this PR, I'd say. @archmoj do you have a case in mind where the existing interpolation is preferable?

[alexcjohnson]

Also closer to what we already do with smoothed heatmaps (the same mock after Plotly.restyle(gd,{'type':'heatmap', 'zsmooth':'best'}) - the four variants look slightly different but I think this is actually a symptom of some funny and potentially incorrect things we're doing with extending beyond the data values to make the outer edges of the heatmap bricks - we may want to adapt this for log axes too, especially as this results in us dropping the first row/column in this case!):

[archmoj]

@archmoj do you have a case in mind where the existing interpolation is preferable?

Here is a codepen to clarify why I think the existing interpolation looks more accurate. The point in the middle (presented by scatter point) should stay on the correct (middle) level. No?

[alexcjohnson]

The point in the middle (presented by scatter point) should stay on the correct (middle) level. No?

You could ask the same question about lines connecting points on a scatter plot - we should connect them with a straight line in data space, then transform that line to log axes where it would be a curve. But that's not what we do: even with log axes we connect with a straight line in pixel space (or, if you've selected spline shape, with a bezier curve calculated in pixel space) and I think displaying the data on log axes is a way of saying that, in general, that's also the space you should be interpolating in. This PR is the equivalent for contours. Perhaps at some point the current behavior could be provided as an option, but to me this PR seems a much better default behavior.

[archmoj]

The point in the middle (presented by scatter point) should stay on the correct (middle) level. No?

You could ask the same question about lines connecting points on a scatter plot - we should connect them with a straight line in data space, then transform that line to log axes where it would be a curve. But that's not what we do: even with log axes we connect with a straight line in pixel space (or, if you've selected spline shape, with a bezier curve calculated in pixel space) and I think displaying the data on log axes is a way of saying that, in general, that's also the space you should be interpolating in. This PR is the equivalent for contours. Perhaps at some point the current behavior could be provided as an option, but to me this PR seems a much better default behavior.

I sort of agree with this part displaying the data on log axes is a way of saying that, in general, that's also the space you should be interpolating in and I won't protest too much if we want to consider this PR a bug fix :)
Interestingly there is a similar issue in 3D I opened a while ago: #3312.
Perhaps we could/should simply close that one if we want to follow this path.

[archmoj]

I suggest we rename this file to 5985_change.md so that it clearly show up in the Changed section.

[alexcjohnson]

Yeah that makes sense to me - this PR is somewhere between a fix and a change, so let's put it in the somewhat more prominent place (and release it in a minor rather than a patch)

[alexcjohnson]

💃 I'd say this is ready once the changelog entry is moved to change - thanks @andrew-matteson!

FYI I created #5991 to track the heatmap brick edge issue I noted in my earlier comment.