Skip to content

Fix order of heatmap z-data - #1833

Closed
dy wants to merge 1 commit into
plotly:masterfrom
dy:heatmap-coords
Closed

Fix order of heatmap z-data#1833
dy wants to merge 1 commit into
plotly:masterfrom
dy:heatmap-coords

Conversation

@dy

@dy dy commented Jun 28, 2017

Copy link
Copy Markdown
Contributor

Should fix #1825

If I get it right, we have rows layout z: [[x,x,x,x], [x,x,x,x], ...], meaning that to retrieve a value we call z[y][x], not z[x][y].

@etpinard

Copy link
Copy Markdown
Contributor

Hmm. pointNumber should give the z[pointNumber[0]][pointNumber[1]] value.

We'll have to check with if this is consistent with non-gl heatmap and contour.

@dy

dy commented Jun 28, 2017

Copy link
Copy Markdown
Contributor Author

@etpinard

Copy link
Copy Markdown
Contributor

but on

https://github.com/plotly/plotly.js/blob/master/src/traces/heatmap/hover.js#L38

we set

ny = index[1]
nx = index[0]

@dy

dy commented Jun 28, 2017

Copy link
Copy Markdown
Contributor Author

Not sure where do we use pointData.index !== false branch at all, because in every case I could find we calculate nx, ny here, which is basically snapping proper x/y value to bins, which corresponds to z[y][x] data layout and the way data is plotted.

@etpinard

etpinard commented Jul 4, 2017

Copy link
Copy Markdown
Contributor

@dy

dy commented Jul 4, 2017

Copy link
Copy Markdown
Contributor Author

@etpinard yes, these cases are for regular heatmap, switching them to heatmapgl raises the error covered here.

@alexcjohnson

Copy link
Copy Markdown
Collaborator

but on

https://github.com/plotly/plotly.js/blob/master/src/traces/heatmap/hover.js#L38

we set

ny = index[1]
nx = index[0]

@etpinard looks consistent to me?

nx = Math.round(pointData.index[1]);
ny = Math.round(pointData.index[0]);

Anyway, the convention is meant to match the way a matrix is printed:

[
  [1, 2, 3],
  [4, 5, 6],
  [7, 8, 9]
]

so the first index is row (y) and the second index is column (x)

@dfcreative the pointData.index !== false case may be for manually triggering hover - ala https://plot.ly/javascript/hover-events/#triggering-hover-events (note you have to scroll the iframe to see the "go" button)

How about covering the relevant cases with tests, then we don't have to argue about this!

@etpinard etpinard added status: in progress bug something broken labels Jul 10, 2017
@etpinard

etpinard commented Jul 13, 2017

Copy link
Copy Markdown
Contributor

It would be nice to fix this thing in 1.29.0. I'll try wrapping this up tomorrow.

@etpinard

Copy link
Copy Markdown
Contributor

Supersedes by #1884

@etpinard etpinard closed this Jul 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug something broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

heatmapgl cursor popup can't go wider than the plot is tall

3 participants