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.

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 order of heatmap z-data #1833

Closed
wants to merge 1 commit into from
Closed

Fix order of heatmap z-data #1833

wants to merge 1 commit into from

Conversation

dy
Copy link
Contributor

@dy dy commented Jun 28, 2017

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
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
Copy link
Contributor Author

dy commented Jun 28, 2017

@etpinard
Copy link
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
Copy link
Contributor Author

dy commented Jun 28, 2017

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
Copy link
Contributor

etpinard commented Jul 4, 2017

@dy
Copy link
Contributor Author

dy commented Jul 4, 2017

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

@alexcjohnson
Copy link
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
Copy link
Contributor

etpinard commented Jul 13, 2017

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

@etpinard
Copy link
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