Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix contour plot colorscale domain #6625

Merged
merged 15 commits into from
Jul 18, 2023
Merged

Fix contour plot colorscale domain #6625

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
66e7b3c
Fix contour colorscale domain missing zmin/zmax
lvlte Jun 2, 2023
fb7e181
Use consistent colorbar fill levels attributes
lvlte Jun 2, 2023
ae34826
Update some contour mocks
lvlte Jun 3, 2023
29bf5db
Update image baselines
lvlte Jun 3, 2023
e889dd1
Add draft log
lvlte Jun 3, 2023
27d84f1
Colorbar test case : use mock data.
lvlte Jun 6, 2023
736242c
Revert "Update some contour mocks"
lvlte Jun 6, 2023
fa5c3e3
Update image baselines
lvlte Jun 6, 2023
bda830b
Fix contour colorscale domain :
lvlte Jun 23, 2023
807504e
Merge branch 'master' into fix-contour-colorscale-6623
lvlte Jun 28, 2023
9f730ea
Update image baselines
lvlte Jun 28, 2023
aea023c
Update draftlogs/6625_fix.md
lvlte Jun 28, 2023
1bd6571
Fix contour colorscale domain/range
lvlte Jun 29, 2023
af9a1d3
Update contour image baselines
lvlte Jun 29, 2023
aea78dd
Add/Improve contour color map test cases
lvlte Jun 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions draftlogs/6625_fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Fix contour plot colorscale domain (take account of [zmin, zmax] and [cmin, cmax]) [[#6625](https://github.com/plotly/plotly.js/pull/6625)]
2 changes: 1 addition & 1 deletion src/components/colorbar/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ function drawColorBar(g, opts, gd) {
.data(fillLevels);
fills.enter().append('rect')
.classed(cn.cbfill, true)
.style('stroke', 'none');
.attr('style', '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain this change?

Copy link
Contributor Author

@lvlte lvlte Jun 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fills.exit().remove();

var zBounds = zrange
Expand Down
30 changes: 27 additions & 3 deletions src/traces/contour/make_color_map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ module.exports = function makeColorMap(trace) {

var si, i;

if(contours.coloring === 'heatmap') {
var zmin0 = cOpts.min;
var zmax0 = cOpts.max;
var zmin0 = cOpts.min;
var zmax0 = cOpts.max;

if(contours.coloring === 'heatmap') {
for(i = 0; i < len; i++) {
si = scl[i];
domain[i] = si[0] * (zmax0 - zmin0) + zmin0;
Expand Down Expand Up @@ -65,6 +65,30 @@ module.exports = function makeColorMap(trace) {
domain[i] = (si[0] * (nc + extra - 1) - (extra / 2)) * cs + start;
range[i] = si[1];
}

// If zmin/zmax are explicitly set
if(typeof trace._input.zmin === 'number' && typeof trace._input.zmax === 'number') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Consider case where user specifies a narrower z range than that
// of the contours start/end.
if(start <= zmin0) {
domain = domain.filter(function(z) { return z >= zmin0; });
range.splice(0, range.length - domain.length);
}
if(end >= zmax0) {
domain = domain.filter(function(z) { return z <= zmax0; });
range.splice(domain.length, range.length - domain.length);
}

// Make the colorscale fit the z range
if(domain[0] > zmin0) {
domain.unshift(zmin0);
range.unshift(range[0]);
}
if(domain[domain.length - 1] < zmax0) {
domain.push(zmax0);
range.push(range[range.length - 1]);
}
}
}

return Colorscale.makeColorScaleFunc(
Expand Down
Binary file modified test/image/baselines/25.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/airfoil.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/cheater_contour.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/contour_match_edges.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/contour_valid_ses.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/h-colorbar_airfoil.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/shared_coloraxes_contour.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
14 changes: 7 additions & 7 deletions test/jasmine/tests/colorbar_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,19 +529,19 @@ describe('Test colorbar:', function() {
return attrs;
}

var z = [[1, 10], [100, 1000]];
var mock = require('../../image/mocks/contour_transposed');
var z = mock.data[0].z;

var expectedAttrs;
var actualAttrs;

Plotly.newPlot(gd, [{type: 'contour', z: z}])
Plotly.newPlot(gd, mock)
.then(function() {
expectedAttrs = getCBFillAttributes();

return Plotly.newPlot(gd, [{type: 'heatmap', z: z}])
.then(function() {
return Plotly.react(gd, [{type: 'contour', z: z}]);
});
return Plotly.newPlot(gd, [{type: 'heatmap', z: z}]);
})
.then(function() {
return Plotly.react(gd, mock);
})
.then(function() {
actualAttrs = getCBFillAttributes();
Expand Down