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.

Sign up for GitHub

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

Jump to bottom

Bump regl-scatter2d and regl-splom and maintain point-cluster at v3 #5419

Merged
merged 5 commits into from
Jan 19, 2021
Merged

Bump regl-scatter2d and regl-splom and maintain point-cluster at v3 #5419

merged 5 commits into from
Jan 19, 2021

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jan 18, 2021 •
edited
Loading

Initial attempt:
Addressing #897.
So that we could add scattergl, splom and scatterpolargl to strict partial bundles in v2.
cc: #5413

Final result:
Bump regl-scatter2d and regl-splom and maintain point-cluster at v3 to avoid potential impacts.

@plotly/plotly_js
cc: @dy

@archmoj archmoj added this to the NEXT milestone Jan 18, 2021
@archmoj archmoj requested a review from alexcjohnson January 18, 2021 15:27
@archmoj archmoj mentioned this pull request Jan 18, 2021
Merged
archmoj
archmoj commented Jan 18, 2021
View reviewed changes
package.json Outdated
@@ -60,7 +60,7 @@
"@plotly/d3": "^3.5.18",
"@plotly/d3-sankey": "0.7.2",
"@plotly/d3-sankey-circular": "0.33.1",
"@plotly/point-cluster": "^3.1.9",
"@plotly/point-cluster": "git://github.com/plotly/point-cluster.git#47d99d990af3741ee30d74f42c83ba5d6d74b509",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plotly/point-cluster#3

@archmoj archmoj mentioned this pull request Jan 18, 2021
Merged
@dy
Copy link
Contributor

dy commented Jan 18, 2021

Hm, is lod not being used? Afaik it allowed significant perf boost for 1M+ points scatterplot interactions.

@dy
Copy link
Contributor

dy commented Jan 18, 2021

I am not the owner of @plotly/point-cluster (btw it seems that fork was a tmp solution, I could transfer point-cluster to gl-vis), but ideally it would take rewriting binary-search-bounds to avoid Function constructor.

@archmoj archmoj mentioned this pull request Jan 18, 2021
Merged
archmoj
archmoj commented Jan 18, 2021
View reviewed changes
package.json Outdated
@@ -110,8 +110,8 @@
"regl": "^1.6.1",
"regl-error2d": "^2.0.11",
"regl-line2d": "^3.1.0",
"regl-scatter2d": "^3.2.1",
"regl-splom": "^1.0.12",
"regl-scatter2d": "git://github.com/gl-vis/regl-scatter2d.git#14876b1f5af6654405055f7dede02164c052163d",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gl-vis/regl-scatter2d#30

archmoj
archmoj commented Jan 18, 2021
View reviewed changes
package.json Outdated
"regl-scatter2d": "^3.2.1",
"regl-splom": "^1.0.12",
"regl-scatter2d": "git://github.com/gl-vis/regl-scatter2d.git#14876b1f5af6654405055f7dede02164c052163d",
"regl-splom": "git://github.com/gl-vis/regl-splom.git#842cfedbd3a791b6b825895fae291f3906ee4f7d",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gl-vis/regl-splom#20

@archmoj
Copy link
Contributor Author

archmoj commented Jan 18, 2021

Hm, is lod not being used? Afaik it allowed significant perf boost for 1M+ points scatterplot interactions.

I don't see that getting called.
The hover appears to work well on tests as well as this codepen.

@dy
Copy link
Contributor

dy commented Jan 18, 2021

It's not hover but panning on zoomed part of the plot.

alexcjohnson
alexcjohnson approved these changes Jan 18, 2021
View reviewed changes
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you're right, this isn't getting used. If we find any cases where performance is suffering that lod is expected to help, we can investigate fixing the function constructor in binary-search-bounds. 💃

@dy
Copy link
Contributor

dy commented Jan 18, 2021 •
edited
Loading

The difference can be seen here https://gl-vis.github.io/regl-scatter2d/ and https://gl-vis.github.io/regl-scatter2d/no-snapping
The first one is with lod, the second - without (discount points outline rendering tax).
Dropping lod makes big blobs get slow on panning.
Also see gl-vis/regl-scatter2d#30 (comment)

@dy
Copy link
Contributor

dy commented Jan 18, 2021

The level of details is the core points-clustering mechanism made by @mikolalysenko long ago, without it clustering is useless, because all possible points get displayed.

@archmoj archmoj changed the title Drop unused option from point-cluster Bump regl-scatter2d and regl-splom and maintain point-cluster at v3 Jan 19, 2021
@archmoj
Copy link
Contributor Author

archmoj commented Jan 19, 2021

The level of details is the core points-clustering mechanism made by @mikolalysenko long ago, without it clustering is useless, because all possible points get displayed.

To avoid potential impacts here as well as the regl-scatter2d users we maintained the version of point-cluster at v3 here as well as regl-splom and regl-scatter2d until mikolalysenko/binary-search-bounds#8 could be addressed.

alexcjohnson
alexcjohnson approved these changes Jan 19, 2021
View reviewed changes
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 @dy sorry for all the noise, we'll get the proper solution implemented eventually!

@archmoj archmoj merged commit e1705fd into master Jan 19, 2021
@archmoj archmoj deleted the scattergl-no-new-func branch January 19, 2021 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcjohnson alexcjohnson alexcjohnson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@archmoj @dy @alexcjohnson