Plotly.js: stacked histograms have different start/end values on hover

[nicolaskruchten]

https://codepen.io/nicolaskruchten/pen/BaNxYzM

This is weird:

[alexcjohnson]

FWIW this happens with regular split hover as well -

[alexcjohnson]

we are just doing the calculation of number of digits to show in these labels independently for each trace even when they're stacked or grouped so the coordinates should match.

[archmoj]

@alexcjohnson is this a bug?

[alexcjohnson]

Yes, bug. I forget how that calculation works but whatever it does should be shared by all traces in a group or stack.

[antoinerg]

The test added in https://github.com/plotly/plotly.js/commit/ee1a6ba4f8797332301eec4d6c64c130324c55b5 showcases the problem at hand.

The problem comes from the fact that each trace has its own rounding function roundFn which is determined by their trace specific leftGap and rightGap:
https://github.com/plotly/plotly.js/blob/9a75ef0bf6e3c49a96c321c86e3ac4abff9b1697/src/traces/histogram/calc.js#L112-L131

which then sets the hover values that end up being different:
https://github.com/plotly/plotly.js/blob/9a75ef0bf6e3c49a96c321c86e3ac4abff9b1697/src/traces/histogram/calc.js#L176-L177

[antoinerg]

One possible solution is to defer the evaluation of ph0 and ph1 to after we're done with the calc phase of all traces. This is what is implemented in branch fix-4648. It's rather hacky but it seems to solve the issue at hand.

cc @alexcjohnson @archmoj can you have a look at it?

[alexcjohnson]

Ideally things like this belong in crossTraceCalc, though that could also be hacky since histogram shares that with bar. Your solution looks good, it's just better to minimize the work in hover when possible.

[antoinerg]

Ideally things like this belong in crossTraceCalc, though that could also be hacky since histogram shares that with bar.

I forgot that crossTraceCalc is executed after the calc steps of each trace. I could definitely do the work of computing ph0 and ph1 over there. The only downside is that we will have to loop over all the bars again.

Your solution looks good, it's just better to minimize the work in hover when possible.

I understand that we may not want to add latency to something interactive. Computing everything in advance is probably a better approach considering we don't usually have a lot of bars.

Thanks for the review @alexcjohnson

[antoinerg]

Another solution using crossTraceCalc can be found in branch fix-4648-cross. Please let me know what you think and how to improve on it!

cc @alexcjohnson @archmoj

[archmoj]

The https://codepen.io/nicolaskruchten/pen/BaNxYzM is fixed in your branch fix-4648. But it is not fixed by branch fix-4648-cross .

[antoinerg]

The https://codepen.io/nicolaskruchten/pen/BaNxYzM is fixed in your branch fix-4648. But it is not fixed by branch fix-4648-cross .

Actually using the bundle from fix-4648-cross, we only get one item when using the hover compare mode. I'm a bit puzzled as to why. Investigating now. Let me know if you have any hint!

Codepen: https://codepen.io/antoinerg/pen/dyoKMZd

[alexcjohnson]

@antoinerg the approach in fix-4648-cross looks great, though somehow it's getting a mismatch of bin positions - looks like maybe fullTrace._binEdges is for all traces but each trace may start at a different bin? Only Africa, which has the left-most bin, looks correct. Not sure why that would cause problems for getting all the other items but perhaps fixing that will resolve both?
Here the box says 40-something but the cursor is at 50-something... and if you switch to closest mode you can look at all the traces.

[antoinerg]

looks like maybe fullTrace._binEdges is for all traces but each trace may start at a different bin?

You were exactly right @alexcjohnson! Fixed in https://github.com/plotly/plotly.js/commit/374941be778ad4442eac26f3fa69fea267bb83cb

[alexcjohnson]

That'll work! I believe the index is simply offset between traces, so it might be more efficient just to store it once, something like cd[0].t.i0 but that's not a big deal.

[antoinerg]

That'll work! I believe the index is simply offset between traces, so it might be more efficient just to store it once, something like cd[0].t.i0 but that's not a big deal.

Sounds good. In the end, I think I will use my first approach of preparing functions in histogram/calc.js and defer their evaluation until later. However, instead of doing the evaluation in hover, I would do it in the crossTraceCalc step. I think it results in cleaner code.

[antoinerg]

When deferring the evaluation of ph0 and ph1, I encounter an edge case: both leftGap and rightGap can be zero and the rounding function then returns NaN.

@alexcjohnson I think you wrote getBinSpanLabelRound. Is it easy to account for this edge case? Basically passing this test: https://github.com/plotly/plotly.js/commit/c824fa38a1a63ea16632317c026cd53e3ea5d9d6