Closes #3497: Show correct values on hover of scatterplot.

[emtwo]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix

Description

Scatterplots should now "show closest data on hover" by default and also use the correct values for the hover text. This fixes the issues described in the ticket below.

Related Tickets & Documents

#3497

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[kravets-levko]

@emtwo Thanks for reporting this bug and for proposed solution. But in fact the only correct way to deal with this but is to properly aggregate the data, either with query (which is possible) or in visualization itself (which is currently not implemented). This bug also exists in other chart types, and IMO such patches will introduce some code that does not solve the issue, but add some complexity in supporting and (later) refactoring.

@arikfr WDYT?

[emtwo]

@kravets-levko I would agree that it might be nice to aggregate other charts in the visualization itself to help users (or to suggest to them to do the aggregation), but I don't think aggregation is the only correct solution for scatter plots.

I think that scatter plots are a little different than the other plot types. They're meant to show individual data points (e.g. imagine the x-axis is years and y-axis is house prices, there should be a point on the graph for each house price in that year). This way, a trend can be observed in a scatter plot by looking at its shape rather than looking at aggregated values. Whereas, in a chart such as a bar graph, for example, it only makes sense to have one y-value per series.

Unfortunately, it seems that the plotly implementation does show the individual data points (which is correct) but the default hover doesn't show the correct values nor does it show all of them.

[ranbena]

@arikfr any input on this?

[arikfr]

Thanks, @emtwo. I think the issue describe in #3497 is indeed a bug: we should show the correct value.

But this solution introduces a few regressions:

1. https://deploy-preview-3527--redash-preview.netlify.com/queries/36#64: the tooltip should show the percentage value and in brackets the absolute value. But in this PR it shows percentage value twice.
2. https://deploy-preview-3527--redash-preview.netlify.com/queries/69#145: the Pie charts don't render.
3. https://deploy-preview-3527--redash-preview.netlify.com/queries/39#83: it shows different values on hover, although I'm not sure if this is a bug or a fix of a bug in this case 😅

This is what I found from reviewing existing examples we had. I don't know if there aren't more issues. For this reason, I'm really hesitant with this PR, because it seems to solve an edge case while potentially breaking many other (more common) cases :|

[emtwo]

@arikfr thanks for pointing out those issues. I didn't realize we had such comprehensive examples in netlify to test with, that's very useful!

I've adjusted the code so it should have no impact on any other chart type except for scatter and bubble to fix the specific issues in them.

[deecay]

All, I support this PR too.

[emtwo]

@arikfr just wanted to resurface this and see if there are any outstanding issues. Thanks!

[deecay]

@arikfr, @kravets-levko, @ranbena,

I would like to discuss this further. I suspect the issue here is "how many y-values a single x-value can have in one series"? Line, Area, Bar charts are fine with one y-value for one x-value per series. But Scatter, Bubble, Box may have multiple y-values per x-value per series.

But in the code (see below), sourceData is populated with x-value as a key. Therefore we can only have one y-value per x-value.

redash/client/app/visualizations/chart/plotly/prepareDefaultData.js

Line 99 in 944adb9

sourceData.set(x, {

Then, when hover texts are created based on sourceData, all data which share one x-value would have same y-values and other attributes.

Not sure what is the best way to fix this, but just wanted to share my finding.

[wlach]

This is old, but AFAIK it's still a real issue and would be nice to fix. The solution is correct from what I can tell.

[justinclift]

Cool, lets try and get it done then. 😄

[guidopetri]

@emtwo , one more thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

edit: whoops, just saw wlach and Justin's comments above.

[justinclift]

@guidopetri Lets leave this one open for further investigation. 😄

[eradman]

I believe the functionality in this change should now be located in viz-lib/src/visualizations/chart/plotly/prepareLayout.ts.

@emtwo are you able to try again using the latest checkout?