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

Point cluster dev #2499

Merged
merged 27 commits into from Mar 28, 2018

Conversation

Projects
None yet
3 participants
@dy
Copy link
Contributor

commented Mar 26, 2018

supersedes #2493

dy and others added some commits Mar 23, 2018

@etpinard

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

image

looks like you brought in some non-ES5 tokens making our image tests 100% not functional.

dy added some commits Mar 26, 2018

@dy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2018

@etpinard is the test-image build fail caused by scattergl update? Looks like something other causes that.

@etpinard

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

you mean this:

image

see -> #2493 (comment)

etpinard and others added some commits Mar 26, 2018

@etpinard

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

Looks like

image

is also having issues in nw.js


@dfcreative Can you try setting up https://gist.github.com/etpinard/d27a44bd5dbee5490f20 ?

You don't anything docker related. Just make sure to use nw.js version 0.12 -> https://dl.nwjs.io/v0.12.0/

dy added some commits Mar 27, 2018

@@ -106,6 +106,12 @@ function plot(container, subplot, cdata) {
if(options.line && !scene.line2d) scene.line2d = true;
if((options.errorX || options.errorY) && !scene.error2d) scene.error2d = true;

stash.tree = cluster(positions);

if(options.marker) {

This comment has been minimized.

Copy link
@etpinard

etpinard Mar 27, 2018

Member

this should probably be options.marker && count >= TOO_MANY_POINTS like in scattergl/convert.js

@etpinard

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

@dfcreative nice! You got the tests to


I noticed a few things by looking at the diff:

peek 2018-03-27 09-33

  • there's a lot more @flaky scattergl tests. Can you think of any reason what that's the case? This added flakiness might be hidding real problems - as @alexcjohnson noticed before in #2415

  • can you please check that this new regl-scatter2d version is at least as good as the previous in terms of memory consumption?

@etpinard

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

@dfcreative I'm noticing that drag performance slows down after toggling layout.dragmode from 'drag' to 'select' back to 'drag' . Is this expected?

Moreover, with

var gd = document.getElementById('graph') 
var x = []
var y = []
var ms = []
var N = 1e6
var i = 0

for(; i < N; i++) {
  x.push(Math.random())
  y.push(Math.random())
  ms.push(i % 2 ? 20 : 10)
}

console.time('plot')
Plotly.newPlot(gd, [{
  type: 'scattergl',
  mode: 'markers',
  x: x,
  y: y,
  marker: {size: ms}
}], {
  dragmode: 'closest',
  //xaxis: {range: [-0.1, 2.2]},
  //yaxis: {range: [-0.1, 1.1]}
}, {scrollZoom: true})

selection is slower than on master (see https://codepen.io/etpinard/pen/QQBzyV). Is this expected for 1e6 random points?

@etpinard

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

... Oh and we should make sure the new regl-scatter2d works in IE11.

etpinard referenced this pull request Mar 27, 2018

@dy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

scatterpolargl tree logic isn't the same as scattergl

Fixed. Although that affects only data layering order, for polar plots that is a bit less actual.

colors in gl2d_scatter-color-clustering are a little different. Is this on purpose?

That is not clustering, that is adjusted shader behavior in case of no-border gl-vis/regl-scatter2d@023c421. It used to add thin black pixel outline instead of keeping the point color, now that "antialias" has the point color.

flakiness

test-jasmine2 passed or failed sporadically, regardless of changes. According to errors, fullLayout is undefined.

IE11

Thanks, fixed. In fact it supports multiple colors now in IE and works even better.
image

Memory
Selection performance

Working on that.

etpinard and others added some commits Mar 27, 2018

Merge pull request #2503 from plotly/fix-unselected-mo-for-array-mo-t…
…races

Fix scattergl unselected marker opacity for array marker opacity traces
@dy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

Ok, with the latest commit selection should work almost as fast as possible, init time is ok, no tree overcalculation and observable memory leaks.

"mapbox-gl": "0.44.1",
"math-log2": "^1.0.1",

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Mar 27, 2018

Contributor

did this get used?

This comment has been minimized.

Copy link
@dy

dy Mar 27, 2018

Author Contributor

Whoops.

@etpinard

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

Well this is amazing! Performance off the latest commit is nothing short of spectacular. Great job @dfcreative 🎉 All my concerns have been addressed.

Now, would you mind publishing a major version of regl-scatter2d (that's v3.0.0 I think) and point-cluster and update the package.json and package-lock.json accordingly? Once that's done, we'll merge this thing right up and move on the sploms 🚀

@etpinard

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

Ok. Well-earned 💃 Let's merge this thing.

@dy dy merged commit d3ff96b into master Mar 28, 2018

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details

@dy dy deleted the point-cluster-dev branch Mar 28, 2018

@etpinard etpinard referenced this pull request Mar 28, 2018

Merged

Introducing splom traces #2505

@dy dy referenced this pull request Dec 11, 2018

Closed

Plotly issue 3232 min #16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.