Sort transform by etpinard · Pull Request #1609 · plotly/plotly.js

etpinard · 2017-04-19T14:53:31Z

Introducing a sort transform which can be very useful for example to sort marker points in descending order:

etpinard · 2017-04-19T14:53:43Z

etpinard · 2017-04-19T14:54:25Z

+    },
+    order: {
+        valType: 'enumerated',
+        values: ['ascending', 'descending'],


I chose ascending and descending to be familiar with axis categoryorder.

etpinard · 2017-04-19T14:56:17Z

+    return function(v) { return +v; };
+}
+
+function getIndices(opts, targetArray, d2c) {


Challenge to all plotly.js contributors: find a faster sort algorithm that this O(n^2) attempt. Note that binary search is nice, but doesn't work so great for arrays with duplicate items.

cc @rreusser @monfera

Not sure I quite understand. For the overall sort? Can you use something like stable? 1. assemble [{value: 2, index: 0}, {value: 5, index: 1}, ...]. 2. Apply stable sort. 3. collect sorted indices and reorder other data arrays by that.

Stable sorting may be useful, e.g. the user presorted based on some criteria, we'd keep that, even if this new facility will support sorting based on multiple fields. Stable sorting is simple with the built-in Array.prototype.sort as in this recently merged thing. The built-in sort is rather fast.

The function d2c seems to always be getDataToCoordFunc which looks a bit expensive. Since it's a lot of calls to this function - 2 calls on each comparison - there's probably benefit to first map (e.g. in a loop) to get the d2c values for all items, and then sort based on that. One way is sorting value/index pairs as above, or more simply, just the indices in a numeric array. Another is to add the d2c value as a property to the objects to be sorted, and then use return a.d2cValue - b.d2cValue || a.originalIndex - b.originalIndex similar to the above link in which case the objects themselves are easily sorted (using the accessor is very fast).

Thanks @monfera , that .map(d2c) pass pre-.sort sounds like a good idea 🐎

^^ put in a bit more code for copy/paste into console in isolation and fixed a typo

^^ another fix and using actual objects to verify stability of the sorting

@monfera how does ^^ handle duplicate items?

Some interesting results in https://gist.github.com/etpinard/86949a01309d5505fab8ccca2b0cd875

Looks like my dumb attempt isn't that bad after all (barring any typos ...)

etpinard · 2017-04-19T14:57:14Z

+};
+
+// TODO reuse for filter.js
+function getTargetArray(trace, target) {


Thinking about putting this in src/lib/. Thoughts?

etpinard · 2017-04-19T14:57:40Z

+}
+
+// TODO reuse for filter.js
+function getDataToCoordFunc(gd, trace, target, targetArray) {


Thinking about putting this in src/plots/cartesian/. Thoughts?

sure, there's nothing explicitly cartesian about this but that's where we have all our axis logic, makes more sense there than hiding in a transform. I guess this says at some point we might want to make a src/components/axes/ or something... but that's a long discussion and even longer project.

we might want to make a src/components/axes/

I like to think of components as optional, i.e. things that could be left out of plotly.js/lib/core.js for users that want to make custom bundle w/o them to shave off some 🍔. As we can't (right?) make the axes logic optional, so I'd vote 👎 for making axes a component. But agreed, most the axes logic isn't strictly cartesian. Maybe it's time for making a src directory e.g. called src/base/ or src/common/ or src/plots/common/ for non-src/lib/ things that are used across multiple plot types?

- use it in filter and sort modules

- use it filter and sort modules.

alexcjohnson · 2017-05-09T15:44:41Z

+    if(typeof target === 'string' && target) {
+        var array = lib.nestedProperty(trace, target).get();
+
+        return Array.isArray(array) ? array : [];


Either this should be : false or the one below should be []

I'd probably vote for false and then instead of the if(!len) return; check, do if(!targetArray) return;

That said, this raises the question, in these applications, of what we should do when targetArray isn't the same length as the other associated arrays. Looks like for both, it will truncate (or extend) all of them to the length of targetArray. Is that what we want? Seems right for sort, but if we agree then lets 🔒 it with a test. For filter there's an argument to be made that eg [] and )(, and {} and }{, should be complementary - which they are within targetArray but not with respect to the items beyond it. Maybe that's still what we want, or maybe we don't want the extra complexity in so far as we think this would be incorrect usage with no legitimate use case. I don't have a strong opinion other than that it should be clear how we handle this case.

BTW it took me a minute to recall the motivation for the different cases in this function - can you give the function a docstring?

in these applications, of what we should do when targetArray isn't the same length as the other associated arrays. Looks like for both, it will truncate (or extend) all of them to the length of targetArray. Is that what we want? Seems right for sort, but if we agree

Yep, that's how I designed back when filter was first added.

but if we agree then lets 🔒 it with a test.

Sure thing.

can you give the function a docstring?

good call 👍

done in 2dd1333 , 9c70ee9 and 4f91957

alexcjohnson · 2017-05-09T15:48:01Z

+
+    for(var i = 0; i < arrayAttrs.length; i++) {
+        var np = Lib.nestedProperty(trace, arrayAttrs[i]);
+        var arrayCopy = np.get().slice();


Is it actually necessary that this be a copy? arrayNew is already a different object...

Yeah, good call. We're only reading the values from those arrays here.

- add 📚 - do not make copy of set target arrays - return false if attribute string get does return array

alexcjohnson · 2017-05-10T00:05:41Z

+        expect(out[1].ids).toEqual(['n0', 'n1', 'n2', 'z', 'p1', 'p2', 'p3', undefined, undefined]);
+        expect(out[1].marker.color).toEqual([0.1, 0.2, 0.3, 0.1, 0.2, 0.3, 0.4, undefined, undefined]);
+        expect(out[1].marker.size).toEqual([10, 20, 5, 1, 6, 0, 10, undefined, undefined]);
+    });


Nice tests! 🎉

alexcjohnson · 2017-05-10T00:06:25Z

Looks great! 💃

etpinard · 2017-05-10T16:05:09Z

        .then(done);
    });
+
+    it('does not preserve hover/click `pointNumber` value', function(done) {


@n-riesco @alexcjohnson IMPORTANT: event data pointNumber does not correspond to the array index in gd.data after a sort transform. The same is true for filter transform with preservegaps: false. This is an unfortunate consequence of the current transform pipeline. Using ids become crucial here. That is, as presented in this test case:

var pt = eventData.points[0] pt.fullData.ids[pt.pointNumber] // => is preserved after a sort transform

We could eventually add a originalPointNumber field to the event data to solve this problem. Moreover, we should really add an id field to the hover & click event data and make the ids data array more ubiquitous (it is only available for scatter traces at the moment).

first cut sort transform

0be666f

etpinard added status: in progress feature something new labels Apr 19, 2017

etpinard added this to the 1.27.0 milestone Apr 19, 2017

etpinard commented Apr 19, 2017

View reviewed changes

etpinard added 3 commits April 26, 2017 11:08

Merge branch 'master' into sort-transform

36f7acd

mv getTargetArray to lib/

1a21ad9

- use it in filter and sort modules

mv getDataToCoordFunc in axes.js

823375c

- use it filter and sort modules.

etpinard added status: reviewable and removed status: in progress labels Apr 26, 2017

alexcjohnson reviewed May 9, 2017

View reviewed changes

etpinard added 3 commits May 9, 2017 15:12

make Lib.getTargetArray API more robust

2dd1333

- add 📚 - do not make copy of set target arrays - return false if attribute string get does return array

do not slice array just to read its values in sort loop

9c70ee9

🔒 behavior target arrays are shorter/longer than other arrays

4f91957

alexcjohnson reviewed May 10, 2017

View reviewed changes

add test for sort transform side effects on eventData pointNumber

4e8efe4

etpinard commented May 10, 2017

View reviewed changes

etpinard merged commit 0204845 into master May 10, 2017

etpinard deleted the sort-transform branch May 10, 2017 18:30

Uh oh!

Uh oh!

Conversation

etpinard commented Apr 19, 2017

Uh oh!

etpinard commented Apr 19, 2017

Uh oh!

etpinard Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson commented May 10, 2017

Uh oh!

etpinard May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

etpinard Apr 19, 2017 •

edited

Loading

etpinard Apr 19, 2017 •

edited

Loading

etpinard Apr 25, 2017 •

edited

Loading

etpinard May 9, 2017 •

edited

Loading

etpinard May 10, 2017 •

edited

Loading