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

Contour bug #1309

Merged
merged 4 commits into from
Jan 18, 2017
Merged

Contour bug #1309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a004e40
fix bug where last contour fill sometimes covered everything
alexcjohnson Jan 18, 2017
4f5de76
standardize the extra we add to contours to avoid rounding errors
alexcjohnson Jan 18, 2017
f140629
test image for the two previous bug fixes
alexcjohnson Jan 18, 2017
ebaaca3
fix and test a bunch more contour edge cases
alexcjohnson Jan 18, 2017
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
3 changes: 2 additions & 1 deletion src/traces/contour/colorbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ var Plots = require('../../plots/plots');
var drawColorbar = require('../../components/colorbar/draw');

var makeColorMap = require('./make_color_map');
var endPlus = require('./end_plus');


module.exports = function colorbar(gd, cd) {
Expand Down Expand Up @@ -52,7 +53,7 @@ module.exports = function colorbar(gd, cd) {
})
.levels({
start: contours.start,
end: contours.end,
end: endPlus(contours),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

colorbar actually adds some more to end for the same purpose - but it turns out not to matter in this case, additional expansion doesn't have any ill effects that I could find, as long as it always ends up at or beyond the level we used to make the colorscale. Sometime though we may want to fold ALL the places we extend a range a little for rounding purposes into the same endPlus mold.

size: cs
})
.options(trace.colorbar)();
Expand Down
18 changes: 18 additions & 0 deletions src/traces/contour/end_plus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Copyright 2012-2017, Plotly, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/


'use strict';

/*
* tiny helper to move the end of the contours a little to prevent
* losing the last contour to rounding errors
*/
module.exports = function endPlus(contours) {
return contours.end + contours.size / 1e6;
};
12 changes: 6 additions & 6 deletions src/traces/contour/find_all_paths.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,13 @@ function makePath(pi, loc, edgeflag) {
if(equalPts(pts[pts.length - 1], pts[pts.length - 2])) pts.pop();
locStr = loc.join(',');

var atEdge = (marchStep[0] && (loc[0] < 0 || loc[0] > n - 2)) ||
(marchStep[1] && (loc[1] < 0 || loc[1] > m - 2)),
closedLoop = (locStr === startLocStr) && (marchStep.join(',') === startStepStr);

// have we completed a loop, or reached an edge?
if((locStr === startLocStr && marchStep.join(',') === startStepStr) ||
(edgeflag && (
(marchStep[0] && (loc[0] < 0 || loc[0] > n - 2)) ||
(marchStep[1] && (loc[1] < 0 || loc[1] > m - 2))))) {
break;
}
if((closedLoop) || (edgeflag && atEdge)) break;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could revert this change actually, as I don't need atEdge separately anymore. It's way more readable now, but a bit faster before as it could short-circuit some of the logic...

Copy link
Contributor

Choose a reason for hiding this comment

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

Your call here. I'm fine with it either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, if we're worried about perf, I should get rid of all those str = pair.join(',') statements - they make the comparison code shorter, but it's way faster to just compare integers twice. Since marchStep.join(',') is in the first clause it would save almost nothing to revert.
#1311


mi = pi.crossings[locStr];
}

Expand Down
5 changes: 3 additions & 2 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 @@ -11,13 +11,14 @@

var d3 = require('d3');
var Colorscale = require('../../components/colorscale');
var endPlus = require('./end_plus');

module.exports = function makeColorMap(trace) {
var contours = trace.contours,
start = contours.start,
end = contours.end,
end = endPlus(contours),
cs = contours.size || 1,
nc = Math.floor((end + cs / 10 - start) / cs) + 1,
nc = Math.floor((end - start) / cs) + 1,
extra = contours.coloring === 'lines' ? 0 : 1;

var scl = trace.colorscale,
Expand Down
9 changes: 6 additions & 3 deletions src/traces/contour/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ var Drawing = require('../../components/drawing');
var heatmapPlot = require('../heatmap/plot');
var makeCrossings = require('./make_crossings');
var findAllPaths = require('./find_all_paths');
var endPlus = require('./end_plus');


module.exports = function plot(gd, plotinfo, cdcontours) {
Expand Down Expand Up @@ -81,9 +82,10 @@ function plotOne(gd, plotinfo, cd) {

function emptyPathinfo(contours, plotinfo, cd0) {
var cs = contours.size,
pathinfo = [];
pathinfo = [],
end = endPlus(contours);

for(var ci = contours.start; ci < contours.end + cs / 10; ci += cs) {
for(var ci = contours.start; ci < end; ci += cs) {
pathinfo.push({
level: ci,
// all the cells with nontrivial marching index
Expand Down Expand Up @@ -163,7 +165,8 @@ function makeFills(plotgroup, pathinfo, perimeter, contours) {
}

function joinAllPaths(pi, perimeter) {
var fullpath = (pi.edgepaths.length || pi.z[0][0] < pi.level) ?
var edgeVal2 = Math.min(pi.z[0][0], pi.z[0][1]),
fullpath = (pi.edgepaths.length || edgeVal2 <= pi.level) ?
'' : ('M' + perimeter.join('L') + 'Z'),
i = 0,
startsleft = pi.edgepaths.map(function(v, i) { return i; }),
Expand Down
17 changes: 11 additions & 6 deletions src/traces/contour/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,26 @@ module.exports = function style(gd) {

var colorMap = makeColorMap(trace);

c.selectAll('g.contourlevel').each(function(d, i) {
c.selectAll('g.contourlevel').each(function(d) {
d3.select(this).selectAll('path')
.call(Drawing.lineGroupStyle,
line.width,
contours.coloring === 'lines' ? colorMap(start + i * cs) : line.color,
contours.coloring === 'lines' ? colorMap(d.level) : line.color,
line.dash);
});

c.selectAll('g.contourbg path')
.style('fill', colorMap(start - cs / 2));
var firstFill;

c.selectAll('g.contourfill path')
.style('fill', function(d, i) {
return colorMap(start + (i + 0.5) * cs);
.style('fill', function(d) {
if(firstFill === undefined) firstFill = d.level;
return colorMap(d.level + 0.5 * cs);
});

if(firstFill === undefined) firstFill = start;

c.selectAll('g.contourbg path')
.style('fill', colorMap(firstFill - 0.5 * cs));
});

heatmapStyle(gd);
Expand Down
Binary file added test/image/baselines/contour_edge_cases.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.
132 changes: 132 additions & 0 deletions test/image/mocks/contour_edge_cases.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
{
"data": [
{
"x": [0, 5],
"y": [0, 4],
"z": [[4, 3], [2, 1]],
"type": "contour",
"contours": {
"start": 1.1,
"end": 4.09,
"size": 1
},
"colorbar": {"x": 0.4, "y": 0.9, "len": 0.2}
},
{
"x": [0, 5],
"y": [0, 4],
"z": [[4, 3], [2, 1]],
"type": "contour",
"contours": {
"start": 1,
"end": 4,
"size": 0.9999999
},
"colorbar": {"x": 1, "y": 0.9, "len": 0.2},
"xaxis": "x2"
},
{
"z": [[0, 0, 0, 0, 0, 0],
[0, 0, 9, 0, 0, 0],
[0, 0, 0, 0, 0, 0],
[0, 0, 0, -9, 0, 0],
[0, 0, 0, 0, 0, 0]],
"type": "contour",
"contours": {
"start": -0.000001,
"end": 0.000001,
"size": 0.000001
},
"colorbar": {"x": 0.4, "y": 0.65, "len": 0.2},
"yaxis": "y2"
},
{
"z": [[0, 0, 0, 0, 0, 0],
[0, 0, 9, -9, 0, 0],
[0, 0, 9, -9, 0, 0],
[0, 0, 9, -9, 0, 0],
[0, 0, 0, 0, 0, 0]],
"type": "contour",
"contours": {
"start": -0.000001,
"end": 0.000001,
"size": 0.000001
},
"colorbar": {"x": 1, "y": 0.65, "len": 0.2},
"xaxis": "x2",
"yaxis": "y2"
},
{
"z": [[0, 0, 0, 0, 0, 0],
[0, 9, 0, 0, 0, 0],
[0, 0, 0, 0, 0, 0],
[0, 0, 0, 0, -9, 0],
[0, 0, 0, 0, 0, 0]],
"type": "contour",
"contours": {
"start": -0.000001,
"end": 0.000001,
"size": 0.000001
},
"colorbar": {"x": 0.4, "y": 0.4, "len": 0.2},
"yaxis": "y3"
},
{
"z": [[0, 0, 0, 0, 0, 0],
[0, 0, -9, 0, 0, 0],
[0, 0, 0, 0, 0, 0],
[0, 0, 0, 9, 0, 0],
[0, 0, 0, 0, 0, 0]],
"type": "contour",
"contours": {
"start": -0.0000005,
"end": 0.000001,
"size": 0.000001
},
"colorbar": {"x": 1, "y": 0.4, "len": 0.2},
"xaxis": "x2",
"yaxis": "y3"
},
{
"z": [[0, 0, 0, 0, 0, 0],
[0, 9, 0, 0, 0, 0],
[0, 0, 0, 0, 0, 0],
[0, 0, 0, 0, -9, 0],
[0, 0, 0, 0, 0, 0]],
"type": "contour",
"contours": {
"start": -0.0000005,
"end": 0.000001,
"size": 0.000001
},
"colorbar": {"x": 0.4, "y": 0.15, "len": 0.2},
"yaxis": "y4"
},
{
"z": [[0, 0, 0, 0, 0, 0],
[0, 0, 9, 0, 0, 0],
[0, -9, -9, -9, -9, 0],
[0, 0, 0, 9, 0, 0],
[0, 0, 0, 0, 0, 0]],
"type": "contour",
"contours": {
"start": -0.0000005,
"end": 0.000001,
"size": 0.000001
},
"colorbar": {"x": 1, "y": 0.15, "len": 0.2},
"xaxis": "x2",
"yaxis": "y4"
}
],
"layout": {
"xaxis": {"domain": [0, 0.4]},
"xaxis2": {"domain": [0.6, 1]},
"yaxis": {"domain": [0.8, 1]},
"yaxis2": {"domain": [0.55, 0.75]},
"yaxis3": {"domain": [0.3, 0.5]},
"yaxis4": {"domain": [0.05, 0.25]},
"width": 600,
"height": 800
}
}