Fix color clustering #2377

dy · 2018-02-16T19:39:47Z

That fixes #2376, #2374 and #2354.
By accident pushed 41c067a to master, this should fix that.

add baseline images

etpinard · 2018-02-16T20:33:10Z

package.json

@@ -100,7 +100,7 @@
    "regl": "^1.3.1",
    "regl-error2d": "^2.0.3",
    "regl-line2d": "^2.1.4",
-    "regl-scatter2d": "^2.1.13",
+    "regl-scatter2d": "^2.1.16",


can you update the package-lock file?

etpinard · 2018-02-16T20:34:47Z

src/traces/scattergl/index.js

            });

-            scene.fill2d.update(scene.fillOptions);
+            scene.fill2d.update(fillOptions);


You forgot to git add the json mock corresponding to gl2d_line_select.png

This is the one that accidentally made it into master, as mentioned above

By accident pushed 41c067a to master, this should fix that.

But since I had that in master as the base of my own PR, I removed it there #2379 (comment)

etpinard · 2018-02-16T20:35:33Z

src/traces/scattergl/index.js

@@ -863,9 +863,11 @@ function plot(container, subplot, cdata) {

                fillOptions.opacity = trace.opacity;
                fillOptions.positions = pos;
+
+                return fillOptions;


Can you briefly explain why this fixes #2376 ?

That fixes #2354. That is internal regl-line2d behavior: when we do line2d.update([opts1, undefined, opts2, ...rest]), the second line in batch is ignored instead of being removed. I made it accept null, indicating that batch item should be removed.

etpinard · 2018-02-16T20:36:41Z

src/traces/scattergl/index.js

@@ -781,9 +781,9 @@ function plot(container, subplot, cdata) {
        }
        // fill requires linked traces, so we generate it's positions here
        if(scene.fill2d) {
-            scene.fillOptions.forEach(function(fillOptions, i) {
+            var fillOptions = scene.fillOptions.map(function(fillOptions, i) {


Can you add a jasmine test to 🔒 this down?

alexcjohnson · 2018-02-16T20:46:56Z

Playing with this branch - looking great up to 90K points!

But then something changes, kind of gradually between 90K and 92K - here's 92K, where it looks like the marker outline colors pick randomly from the marker colors?

Then go a bit farther and the marker colors also get randomized - here's 100K points.

The code I used for this is (basically copied from one of the issue codepens):

var x_array = [], color_array = [], opacity = [], size = [];

for (var i = 0; i < N; i++){ // N = eg 90000, 92000, 100000
  x_array.push(i);
  color_array.push(Math.random());
  opacity.push(Math.random());
  size.push(Math.random() * 3 + 3);
}

var trace1 = {
  x: x_array, y: color_array,
  mode: 'markers',
  marker: {
    size: size, color: color_array, opacity: opacity,
    colorscale: [[0, 'rgb(255, 0, 0)'], [0.5, 'rgb(0, 255, 0)'], [1.0, 'rgb(0, 0, 255)']]
  },
  type: 'scattergl'
};

var layout = {
  title: 'Scatter Plot with a Color Dimension'
};

Plotly.newPlot(gd, [trace1], layout);

dy · 2018-02-19T22:03:32Z

@alexcjohnson that is regl limitation of uint32 buffers, we can't go over 65536 various colors, that is reached at ~91k points.

etpinard · 2018-02-19T22:17:46Z

package-lock.json

@@ -10260,115 +10238,6 @@
        "update-diff": "1.1.0"
      }
    },
-    "regl-scatter2d": {


Why is this gone?

etpinard · 2018-02-19T22:18:27Z

package-lock.json

-      "version": "0.44.0",
-      "resolved": "https://registry.npmjs.org/mapbox-gl/-/mapbox-gl-0.44.0.tgz",
-      "integrity": "sha512-vMeZaLXjG1B1BKOD9HB11sb9UIUvbzXWJu0NR38j9Uyp1h5xUXqh1Rqe+EhxQp3jzlHIv/LVhFKCJjQQKA2LoA==",
+      "version": "0.44.1",


Can we not bump mapbox-gl in this PR?

alexcjohnson · 2018-02-19T22:29:34Z

@alexcjohnson that is regl limitation of uint32 buffers, we can't go over 65536 various colors, that is reached at ~91k points.

Makes sense, but we need to do something reasonable in this case (which should be well within the range that scattergl is expected to support), and not have it break like this. My eyes can't distinguish 65k colors, so we should be able to find a way to discretize the colors to fit within that limit.

dy · 2018-02-20T01:35:29Z

@alexcjohnson fixed by d604a76

alexcjohnson · 2018-02-20T04:07:06Z

@alexcjohnson fixed by d604a76

Beautiful! Here's 1M points now, looks lovely.

Is there an obvious way to test this? I wouldn't want to make a mock with 100k or more points in it, is there a way to trigger the difference at a lower volume? Or perhaps draw something like this programmatically (but not random like in my test) and read a few pixels?

- this locks down commit: gl-vis/regl-scatter2d@6138fd7

alexcjohnson · 2018-02-21T14:54:52Z

test/jasmine/tests/gl2d_plot_interact_test.js

+
+        for(var i = 0; i < N; i++) {
+            x.push(i);
+            color.push(Math.random());


maybe I'm being paranoid, but I still don't like random in tests. Can we do something repeatable like

plotly.js/src/traces/box/plot.js

Lines 17 to 30 in ce5bc97

var randSeed = 2000000000;

function seed() {

randSeed = 2000000000;

}

function rand() {

var lastVal = randSeed;

randSeed = (69069 * randSeed + 1) % 4294967296;

// don't let consecutive vals be too close together

// gets away from really trying to be random, in favor of better local uniformity

if(Math.abs(randSeed - lastVal) < 429496729) return rand();

return randSeed / 4294967296;

}

?

Ha right. Good call!

done via
20c3a00 and eae6da2

alexcjohnson · 2018-02-21T16:15:19Z

src/lib/index.js

+    randSeed = 2000000000;
+};
+
+lib.pseudoRandom = function() {


Great idea - I bet we'll find other places this should be used too.

alexcjohnson · 2018-02-21T16:23:28Z

By accident pushed 41c067a to master, this should fix that.

Merging master 1154589 includes the commit where I 🔪 gl2d_line_select.json, right? I don't see it generated in https://circleci.com/gh/plotly/plotly.js/7082#artifacts/containers/0. So now we need to add it back into this PR. After that this looks great to me! 💃

... that was accidentally pushed to master in 41c067a and removed in 1154589

- partly fixes #2386

etpinard · 2018-02-21T19:39:24Z

Merging master 1154589 includes the commit where I gl2d_line_select.json, right? I

Good 👁️ with commit f8b4e52 we get:

Some weird things happened on CI during this push, so I decided to bump glslify to get clean npm i through. This partly fixes #2386

@dfcreative anything else you may want to add to this PR?

dy · 2018-02-22T02:07:12Z

@etpinard so far looks great, thanks a lot for helping out! 💃

dy added 3 commits February 16, 2018 14:37

Fix color clustering

5de6918

Add line select image

ce01ea7

Fix fill update

20559c0

dy mentioned this pull request Feb 16, 2018

scattergl issues with opacity and clustering #2374

Closed

Add clustering baseline

7c944c9

dy requested a review from etpinard February 16, 2018 20:07

etpinard reviewed Feb 16, 2018

View reviewed changes

etpinard added bug something broken status: reviewable labels Feb 16, 2018

etpinard reviewed Feb 16, 2018

View reviewed changes

alexcjohnson mentioned this pull request Feb 17, 2018

Generalize spikelines #2379

Merged

Update deps

4fada56

etpinard reviewed Feb 19, 2018

View reviewed changes

package-lock.json Outdated

@@ -10260,115 +10238,6 @@

"update-diff": "1.1.0"

}

},

"regl-scatter2d": {

Copy link

Contributor

etpinard Feb 19, 2018

Choose a reason for hiding this comment

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

Why is this gone?

etpinard reviewed Feb 19, 2018

View reviewed changes

dy added 2 commits February 19, 2018 17:23

Merge branch 'master' into regl-scatter-fix

87b22b2

Update package-lock

64f112f

dy added 4 commits February 19, 2018 17:42

Make direct fill update

f2e80a7

Merge branch 'master' into regl-scatter-fix

1154589

Bump regl-scatter2d

d604a76

Lock clearing fill & fix selection

9532210

add px-average test scattergl marker.color

7cb3295

- this locks down commit: gl-vis/regl-scatter2d@6138fd7

etpinard force-pushed the regl-scatter-fix branch from 21ce74e to 7cb3295 Compare February 20, 2018 23:13

fixup (no need to create graph div in test)

ba40db9

alexcjohnson reviewed Feb 21, 2018

View reviewed changes

etpinard added 2 commits February 21, 2018 10:45

move pseudo-random seeder and generator to Lib

20c3a00

use pseudo-random number in scattergl large color arrays test

eae6da2

alexcjohnson reviewed Feb 21, 2018

View reviewed changes

etpinard added 3 commits February 21, 2018 13:54

bring back gl2d_line_select mock

f8b4e52

... that was accidentally pushed to master in 41c067a and removed in 1154589

bump glslify to 6.1.1

48e1124

- partly fixes #2386

[fixup] bring back mapbox-gl@0.44.0

0e746bf

etpinard merged commit 1256c4f into master Feb 22, 2018

etpinard deleted the regl-scatter-fix branch February 22, 2018 14:46

etpinard mentioned this pull request Feb 22, 2018

scattergl fill - autorange and restyle #2354

Closed

alexcjohnson mentioned this pull request Feb 22, 2018

Remove palette, replace with direct colors buffer gl-vis/regl-scatter2d#10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix color clustering #2377

Fix color clustering #2377

dy commented Feb 16, 2018 •

edited

Loading

etpinard Feb 16, 2018

dy Feb 19, 2018

etpinard Feb 16, 2018

alexcjohnson Feb 17, 2018

etpinard Feb 16, 2018 •

edited

Loading

dy Feb 19, 2018 •

edited

Loading

etpinard Feb 16, 2018

alexcjohnson commented Feb 16, 2018

dy commented Feb 19, 2018 •

edited

Loading

etpinard Feb 19, 2018

etpinard Feb 19, 2018

alexcjohnson commented Feb 19, 2018

dy commented Feb 20, 2018

alexcjohnson commented Feb 20, 2018

alexcjohnson Feb 21, 2018

etpinard Feb 21, 2018

etpinard Feb 21, 2018

alexcjohnson Feb 21, 2018

alexcjohnson commented Feb 21, 2018

etpinard commented Feb 21, 2018 •

edited

Loading

dy commented Feb 22, 2018

	var randSeed = 2000000000;

	function seed() {
	randSeed = 2000000000;
	}

	function rand() {
	var lastVal = randSeed;
	randSeed = (69069 * randSeed + 1) % 4294967296;
	// don't let consecutive vals be too close together
	// gets away from really trying to be random, in favor of better local uniformity
	if(Math.abs(randSeed - lastVal) < 429496729) return rand();
	return randSeed / 4294967296;
	}

Fix color clustering #2377

Fix color clustering #2377

Conversation

dy commented Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

dy Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Feb 16, 2018

dy commented Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Feb 19, 2018

dy commented Feb 20, 2018

alexcjohnson commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Feb 21, 2018

etpinard commented Feb 21, 2018 • edited Loading

dy commented Feb 22, 2018

dy commented Feb 16, 2018 •

edited

Loading

etpinard Feb 16, 2018 •

edited

Loading

dy Feb 19, 2018 •

edited

Loading

dy commented Feb 19, 2018 •

edited

Loading

etpinard commented Feb 21, 2018 •

edited

Loading