Filter-groupby interaction #1892

rreusser · 2017-07-18T18:01:58Z

This PR adds a failing test for filter + groupby. The goal is to rework this into a PR that solves the issue. That may or may not be possible due to the way one is a calcTransform and the other is a transform. I'll dig in and see if I can figure out what it would take to fix this.

See: #1843

rreusser · 2017-07-25T20:49:47Z

@alexcjohnson @etpinard I added some more challenging tests for transforms. The actual change to groupby was smaller than I expected (and deserves a close look).

I implemented recursive transform application to lock down the way transforms are recursively expanded and iterated, but I wasn't actually able to come up with a test case that it solves, so I removed it. It might be worth keeping in mind though since I suspect it might be a more robust/extensible way to lock down application of transforms.

etpinard · 2017-07-26T15:22:31Z

src/transforms/groupby.js

+// destination. This function does not break the array reference
+// connection between the split transforms it creates. That's handled in
+// initialize, which creates a new empty array for each arrayAttr.
+function cloneTransforms(newTrace) {


Interesting fix 👍

@rreusser is that something we should do for all transforms?

Oh hah - of course, because the original Lib.extendDeepNoArrays({}, trace) doesn't dive into the transforms array. Do we need a Lib.extendDeepOnlyTheseArrays({}, trace, ['transforms']) ? 🙈

is that something we should do for all transforms?

This only applies to transforms that generate multiple traces, right? I do kind of like the idea of recursive transforms, but until we do something like that, that might be more inherently robust, I think we should manage this case-by-case.

So… arrayAttrs does pick up on data_array arrays nested inside transforms. The particular challenge is executing a transform after another has created expanded traces. Because if the first transform splits it into multiple traces, then each expanded trace needs its own copy of the other transforms since they, for example, have different split target or groups data. By expanding deep with no arrays and then cloning the data arrays (which are in arrayAttrs, they all have their own copy that can be filtered appropriately.

Oh, I missed a bit of subtlety. Let me check on the identity of the split transforms. Unless I'm mistaken, something somewhere is cloning those… Let me dig that up.

@etpinard I wondered the same. The downfall of transforms at the moment is that there are too many corner cases and different things transforms might need to do to lock them down to a completely airtight state such that they can only do what they need to do. The result is that there are some patterns like this that should not be a concern of individual transforms at all but which might need to be applied to them all separately. Or if a clear pattern arises, then it can be abstracted. At the moment, the sample size is just a bit to small for me to see overarching patterns.

Or if a clear pattern arises, then it can be abstracted. At the moment, the sample size is just a bit to small for me to see overarching patterns.

Very good answer. Thanks!

@alexcjohnson Never mind. I read my code more carefully. I'm just manually transferring the transforms. 👍

alexcjohnson · 2017-07-26T15:42:57Z

src/transforms/groupby.js

            if(groups[j] !== groupName) continue;

-            arrayAttrs.forEach(pasteArray.bind(0, newTrace, trace, j));
+            arrayAttrs.forEach(pasteArray.bind(null, newTrace, trace, j));


This isn't new here - except that the change you made reminded me how bind always confuses me - but it occurs to me that there's no reason to use it at all if we unwind the forEach to a for. Also pasteArray is a bit inefficient in the way it calls Lib.nestedProperty(newTrace, a) twice.

🎉 🎉 🎉 Glad to change 😄

@alexcjohnson @etpinard I've refactored… actually a fair amount of groupby. It was mostly trivial reordering and inlining in order to cut down on function calls and slice and concat operations, just to streamline things a bit function-call and memory-wise.

Oh, and also it was iterating over all points for each group, which is unnecessary. Now it just iterates over all points once and uses an index to pick the right expanded trace destination.

rreusser · 2017-07-26T18:07:08Z

@monfera's groupby tests saved me here. Refactored, optimized, and fixed some test cases. I'm content. 👍

alexcjohnson · 2017-07-26T18:26:44Z

src/transforms/groupby.js

+
+        // Initialize empty arrays for the arrayAttrs, to be split in the next step
+        for(j = 0; j < arrayAttrs.length; j++) {
+            Lib.nestedProperty(newTrace, arrayAttrs[j]).set([]);


nice - what you're doing here is pretty similar in fact to what I'm doing with the aggregate transform 🎉

I suspect you could speed it up even a bit more by hanging onto these arrays as you create them, and avoid the inner-loop Lib.nestedProperty calls below

Yeah, that was bugging me, though I know the effect is small in the grand scheme of things. You know exactly what you'll need, so I just indexed them directly and then looked them up by group index. It's still lots of nestedProperty instantiation, but unless we get fancy (read: fragile), I think this is somewhat minimal for the task at hand.

rreusser · 2017-07-26T18:56:45Z

src/transforms/groupby.js


    for(i = 0; i < groupNames.length; i++) {
        groupName = groupNames[i];
+        groupIndex[groupName] = i;


Naming is tricky. It's actually the array index that I'm indexing. So it should be called indexIndex 😕

indexLookup. Better, maybe. A variable by any other name would smell… as… hmm… 🌹

alexcjohnson · 2017-07-26T19:01:32Z

Looks great! 💃

rreusser · 2017-07-26T19:05:51Z

I'll defer to someone else on whether this is a bugfix or a feature. Since the output for interactions was simply incorrect/nonsensical before, I'd tend to consider this a bugfix.

etpinard · 2017-07-26T19:12:16Z

I'd tend to consider this a bugfix.

Yeah. This is a 🪲 fix for sure.

Add failing test for filter-groupby interaction

84164e3

rreusser added the status: in progress label Jul 18, 2017

rreusser added 5 commits July 25, 2017 13:01

Make transforms apply recursively

9373680

Remove transform hack

9b44b4b

Merge branch 'master' into filter-groupby-combo

464b18a

Add group + group test and revert src/plots

7ebee7d

Add groupby + groupby + filter test

c742487

rreusser added 2 commits July 25, 2017 13:51

Typo fix in groupby comments

5668830

Add filter + filter test

695ad68

rreusser changed the title ~~[DO NOT MERGE] Add failing test for filter-groupby interaction~~ Filter-groupby interaction Jul 25, 2017

rreusser added status: reviewable and removed status: in progress labels Jul 25, 2017

etpinard reviewed Jul 26, 2017

View reviewed changes

alexcjohnson reviewed Jul 26, 2017

View reviewed changes

rreusser added 2 commits July 26, 2017 09:59

Optimize and refactor groupby

1544e71

Style groups *after* computing data

9f0600a

alexcjohnson reviewed Jul 26, 2017

View reviewed changes

Avoid extra nestedProperty creation in groupby

364f8f7

rreusser commented Jul 26, 2017

View reviewed changes

Select less deliberately confusing name for variable

88b5673

alexcjohnson added the bug something broken label Jul 26, 2017

rreusser merged commit 658a096 into master Jul 26, 2017

rreusser deleted the filter-groupby-combo branch July 26, 2017 19:11

rreusser mentioned this pull request Jul 26, 2017

Filter + groupby breaks if filter includes target data #1843

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter-groupby interaction #1892

Filter-groupby interaction #1892

rreusser commented Jul 18, 2017

rreusser commented Jul 25, 2017 •

edited

Loading

etpinard Jul 26, 2017

etpinard Jul 26, 2017

alexcjohnson Jul 26, 2017

rreusser Jul 26, 2017

rreusser Jul 26, 2017

rreusser Jul 26, 2017 •

edited

Loading

etpinard Jul 26, 2017

rreusser Jul 26, 2017

alexcjohnson Jul 26, 2017

rreusser Jul 26, 2017

rreusser Jul 26, 2017 •

edited

Loading

rreusser commented Jul 26, 2017 •

edited

Loading

alexcjohnson Jul 26, 2017

rreusser Jul 26, 2017

rreusser Jul 26, 2017

rreusser Jul 26, 2017 •

edited

Loading

rreusser Jul 26, 2017

alexcjohnson commented Jul 26, 2017

rreusser commented Jul 26, 2017

etpinard commented Jul 26, 2017

Filter-groupby interaction #1892

Filter-groupby interaction #1892

Conversation

rreusser commented Jul 18, 2017

rreusser commented Jul 25, 2017 • 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

Choose a reason for hiding this comment

rreusser Jul 26, 2017 • 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

Choose a reason for hiding this comment

rreusser Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

rreusser commented Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Jul 26, 2017

rreusser commented Jul 26, 2017

etpinard commented Jul 26, 2017

rreusser commented Jul 25, 2017 •

edited

Loading

rreusser Jul 26, 2017 •

edited

Loading

rreusser Jul 26, 2017 •

edited

Loading

rreusser commented Jul 26, 2017 •

edited

Loading

rreusser Jul 26, 2017 •

edited

Loading