Skip to content

Mulitple fixes for multi-heatmap-trace plots #655

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

Merged
merged 7 commits into from
Jun 20, 2016
Merged

Mulitple fixes for multi-heatmap-trace plots #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2ef4411
update only varying fields in merge selection
etpinard Jun 15, 2016
90d7a94
make sure off-srceen heatmap traces are disposes
etpinard Jun 15, 2016
48dc191
require MAXDIST from cartesian constants not Fx
etpinard Jun 16, 2016
221a5a1
add multi-trace heatmap mock
etpinard Jun 16, 2016
5084daf
add heatmap plot and hover tests
etpinard Jun 16, 2016
897cb6f
Merge branch 'master' into heatmap-multi-trace
etpinard Jun 20, 2016
7a2dc05
Merge branch 'master' into heatmap-multi-trace
etpinard Jun 20, 2016
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
12 changes: 8 additions & 4 deletions src/traces/heatmap/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@
var Fx = require('../../plots/cartesian/graph_interact');
var Lib = require('../../lib');

var MAXDIST = require('../../plots/cartesian/constants').MAXDIST;


module.exports = function hoverPoints(pointData, xval, yval, hovermode, contour) {
// never let a heatmap override another type as closest point
if(pointData.distance < Fx.MAXDIST) return;
if(pointData.distance < MAXDIST) return;

var cd0 = pointData.cd[0],
trace = cd0.trace,
Expand Down Expand Up @@ -46,8 +48,8 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode, contour)
return;
}
}
else if(Fx.inbox(xval - x[0], xval - x[x.length - 1]) > Fx.MAXDIST ||
Fx.inbox(yval - y[0], yval - y[y.length - 1]) > Fx.MAXDIST) {
else if(Fx.inbox(xval - x[0], xval - x[x.length - 1]) > MAXDIST ||
Fx.inbox(yval - y[0], yval - y[y.length - 1]) > MAXDIST) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This bug was pretty painful to track down.

It was introduced in d7abd18 when some graph_interact.js constants got moved to plots/cartesian/constants.js.

Unfortunately, Fx.MAXDIST was still being used in heatmap hover which was undefined.

Reminder: don't forgot to grep/ack/ag when moving things around!

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry about that @etpinard - as a bit of karmic penance, I fixed this myself in #673 then had to undo my fix when I merged master and got your fix too!

return;
}
else {
Expand All @@ -69,10 +71,12 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode, contour)
nx = Math.max(0, Math.min(x2.length - 2, Lib.findBin(xval, x2)));
ny = Math.max(0, Math.min(y2.length - 2, Lib.findBin(yval, y2)));
}

var x0 = xa.c2p(x[nx]),
x1 = xa.c2p(x[nx + 1]),
y0 = ya.c2p(y[ny]),
y1 = ya.c2p(y[ny + 1]);

if(contour) {
x1 = x0;
xl = x[nx];
Expand All @@ -99,7 +103,7 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode, contour)
return [Lib.extendFlat(pointData, {
index: [ny, nx],
// never let a 2D override 1D type as closest point
distance: Fx.MAXDIST + 10,
distance: MAXDIST + 10,
x0: x0,
x1: x1,
y0: y0,
Expand Down
39 changes: 24 additions & 15 deletions src/traces/heatmap/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,24 @@ function plotOne(gd, plotinfo, cd) {
var imageWidth = Math.round(right - left),
imageHeight = Math.round(bottom - top);

// now redraw
// setup image nodes

// if image is entirely off-screen, don't even draw it
if(imageWidth <= 0 || imageHeight <= 0) return;
var isOffScreen = (imageWidth <= 0 || imageHeight <= 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heatmap.plot was returning early for off screen traces, but did not clear the exiting nodes.


var plotgroup = plotinfo.plot.select('.imagelayer')
.selectAll('g.hm.' + id)
.data(isOffScreen ? [] : [0]);

plotgroup.enter().append('g')
.classed('hm', true)
.classed(id, true);

plotgroup.exit().remove();

if(isOffScreen) return;

// generate image data

var canvasW, canvasH;
if(zsmooth === 'fast') {
Expand Down Expand Up @@ -363,26 +377,21 @@ function plotOne(gd, plotinfo, cd) {
gd._hmpixcount = (gd._hmpixcount||0) + pixcount;
gd._hmlumcount = (gd._hmlumcount||0) + pixcount * avgColor.getLuminance();

var plotgroup = plotinfo.plot.select('.imagelayer')
.selectAll('g.hm.' + id)
.data([0]);
plotgroup.enter().append('g')
.classed('hm', true)
.classed(id, true);
plotgroup.exit().remove();

var image3 = plotgroup.selectAll('image')
.data(cd);
image3.enter().append('svg:image');
image3.exit().remove();

image3.attr({
image3.enter().append('svg:image').attr({
xmlns: xmlnsNamespaces.svg,
'xlink:href': canvas.toDataURL('image/png'),
preserveAspectRatio: 'none'
});

image3.attr({
height: imageHeight,
width: imageWidth,
x: left,
y: top,
preserveAspectRatio: 'none'
y: top
});

image3.exit().remove();
}
Binary file added test/image/baselines/heatmap_multi-trace.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.
193 changes: 193 additions & 0 deletions test/image/mocks/heatmap_multi-trace.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
{
"data": [
{
"x": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We didn't have a mock for a subplot with multiple heatmap traces on it yet. We have numerous multiple subplots mocks with one heatmap per subplot, but none for multiple heatmap on the same x/y subplot. So, here's one.

1,
1,
1,
1,
1,
1
],
"y": [
0,
1,
2,
3,
4,
5
],
"z": [
3,
4,
2,
5,
3,
1
],
"type": "heatmap",
"showlegend": false,
"showscale": false,
"zmax": 5,
"zmin": 1
},
{
"x": [
2,
2,
2,
2,
2
],
"y": [
0.2,
1.2,
2.2,
3.2,
4.2
],
"z": [
6,
1,
3,
2,
4
],
"type": "heatmap",
"showlegend": false,
"showscale": false,
"zmax": 6,
"zmin": 1
},
{
"x": [
3,
3,
3,
3,
3,
3,
3,
3,
3,
3,
3
],
"y": [
0,
0.5,
1,
1.5,
2,
2.5,
3,
3.5,
4,
4.5,
5
],
"z": [
6,
1,
3,
2,
4,
2,
7,
4,
3,
2,
5
],
"type": "heatmap",
"showlegend": false,
"showscale": false,
"zmax": 7,
"zmin": 1
},
{
"x": [
4,
4,
4,
4,
4,
4
],
"y": [
0,
0.2,
0.5,
1.2,
3.5,
4
],
"z": [
2,
4,
7,
3,
2,
1
],
"type": "heatmap",
"showlegend": false,
"showscale": false,
"zmax": 7,
"zmin": 1
},
{
"x": [
5.2,
5.2,
5.2,
5.2,
5.2,
5.2
],
"y": [
0,
0.2,
0.5,
1.2,
3.5,
4
],
"z": [
2,
4,
7,
3,
2,
1
],
"type": "heatmap",
"showlegend": false,
"showscale": true,
"zmax": 10,
"zmin": 0
}
],
"layout": {
"hovermode": "closest",
"xaxis": {
"type": "linear",
"range": [
0.5,
5.7
],
"autorange": true
},
"yaxis": {
"type": "linear",
"range": [
-0.5,
5.5
],
"autorange": true
},
"height": 450,
"width": 1100,
"autosize": true
}
}
Loading