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
etpinard 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 numberDiff line numberDiff 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 DownExpand 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) {

@etpinardetpinardJun 16, 2016

Copy link
Copy Markdown
ContributorAuthor

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!

@alexcjohnsonalexcjohnsonJun 21, 2016

Copy link
Copy Markdown
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 numberDiff line numberDiff 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);

@etpinardetpinardJun 16, 2016

Copy link
Copy Markdown
ContributorAuthor

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 DownExpand 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 addedtest/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 numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
{
"data": [
{
"x": [

@etpinardetpinardJun 16, 2016

Copy link
Copy Markdown
ContributorAuthor

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