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

No scattergl clustering below 1e5 pts #3578

Merged
merged 4 commits into from
Feb 25, 2019

Conversation

etpinard
Copy link
Contributor

An attempts at fixing #2334 by skipping point clustering (done by point-cluster) for scattergl traces with less than 1e5 pts.

Previously, we were subject to the regl-scatter2d default value of 1e4 meaning scattergl traces with more than 1e4 pts but less than 1e5 pts (our TOO_MANY_POINTS constant) were clustered from regl-scatter2d whereas traces with more than 1e5 pts were clustered from Scattergl.calc.

With the PR, I noticed that panning is slightly slower for graphs 1e4 < pts < 1e5, but otherwise performance appears the same. I think this small performance lost is worth it to get the correct rendering and to make us call point-cluster from one place i.e. from Scattergl.calc.

Here are some updated codepens from #2334:

We may eventually still want to lower the maxDepth clustering option as @dy has suggested in #2334 (comment) to potentially correct rendering of traces with > 1e5 pts, but I'd like to wait to find a currently failing example before doing so. We could also try to bring back clustering for 1e4 < pts < 1e5 traces together with a lower maxDepth down the road.

cc @plotly/plotly_js I'd like to slide to in the 1.45.0 - as this PR may have consequences I don't want to release in a patch version.

- note that to do so for 1e4 < pts < 1e5, we must set
  the regl-scatter2d "snap" option too 1e5 (aka TOO_MANY_POINTS)
  as regl-scatter2d uses a dflt value of 1e4.
... now featuring a better rendering.
@etpinard etpinard added bug something broken status: reviewable labels Feb 25, 2019
@etpinard etpinard added this to the v1.45.0 milestone Feb 25, 2019
@archmoj
Copy link
Contributor

archmoj commented Feb 25, 2019

Awesome fix!
Before
After
Thanks @etpinard.
Concerning too many line numbers in gl2d_no-clustering.json and also in order to improve the mock and baseline test with points with opacity, would you please replace it with the data printed to console here?

@etpinard
Copy link
Contributor Author

etpinard commented Feb 25, 2019

@archmoj I like the way gl2d_no-clustering.json easily demonstrated the fix (i.e. one pt was missing, now it is not), so instead of replacing it, I simply added a new mock in f6e8918

I hope you don't mind.

@archmoj
Copy link
Contributor

archmoj commented Feb 25, 2019

Excellent! Thanks.
💃

@etpinard etpinard merged commit 3e15f17 into master Feb 25, 2019
@etpinard etpinard deleted the scattergl-no-clustering-below-1e5-pts branch February 25, 2019 23:25
@deto deto mentioned this pull request Mar 7, 2019
etpinard added a commit that referenced this pull request Sep 23, 2019
- post #3578 all scattergl traces either set a `tree` or
  and `ids` array during the calc step.
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.

2 participants