[WIP] Transforms [rebase of timelyportfolio's work] #936

monfera · 2016-09-14T08:20:48Z

Rebase and lint of @timelyportfolio's PR #859
Issue #917

…ster `filter` with plotly

monfera · 2016-09-14T08:59:41Z

PS. never mind, they pass after registering the transforms, I guess these should eventually be done centrally right under src.

Currently, tests pass because some tests are commented out for now. @timelyportfolio @etpinard did the commented out tests use to pass? (I mean in the feature branch rather than master where the tests are also present)

timelyportfolio · 2016-09-14T11:18:51Z

Like this ?https://github.com/timelyportfolio/plotly.js/blob/9b65721f645c3fd1a9c17c0ceca7a9c29234e30a/lib/index.js#L32-L35

We left groupby out since was not fully fleshed out, but I believe the intention was to register by default also.

monfera · 2016-09-14T11:28:38Z

@timelyportfolio thanks, exactly, probably sticking it in the big list unless @etpinard you think otherwise (there may have been a reason to not include it in the feature branch)

monfera · 2016-09-14T13:53:34Z

@etpinard (not transform related question) I just noticed that if I supply a scalar e.g. y: 10, then it'll ignore it and just use a running sequence of [0, 1, .., x.length - 1] - I guess it's intentional.

(Current groupby chokes if I don't supply an array for y which is all well as in the 1st round I'm just going around making breaking test cases, then bring in compliance)

etpinard · 2016-09-14T15:38:16Z

@monfera don't worry about the current groubpy transform in test/jasmine/assets/.

That file was only meant for testing purposes.

rreusser · 2016-09-14T16:59:10Z

src/transforms/filter.js

        dflt: '='
    },
    value: {
-        valType: 'number',
+        valType: 'any',
        dflt: 0
    },
    filtersrc: {


Can we add ids (edit: not identifiers) to the filtersrc enum? The previous blocker was the animation PR getting merged, but now that it's merged, it's probably a good idea. Without that, I believe @timelyportfolio had to use floating point equaity in order to use the lasso selector and then filter to the selected set. Since in already uses indexOf, I think this perhaps really is a one-liner.

you mean ids ?

Thank you, yes. I was looking at the wrong branch. ids.

etpinard · 2016-09-15T15:50:15Z

src/transforms/filter.js

+            return v;
+        });
+    } else {
+        if(isNumeric(transformOut.value)) transformOut.value = +transformOut.value;


I would prefer moving the array item type conversion down to the transform step - supplyDefaults (even the transform version) should try to avoid diving into arrays.

FYI I haven't touched anything in filter.js so it's now as it is in @timelyportfolio's branch (and I haven't yet pushed the changes to groupby.js. If I understand the task correctly, I need to work on groupby as part of the first sprint on it so I'll get back on it in a bit.

(cherry picked from commit 7e967fe)

…f more cases inside

etpinard · 2016-09-22T15:25:41Z

src/plot_api/crawler.js

+// list of underscore attributes to keep in schema as is
+crawler.UNDERSCORE_ATTRS = [crawler.IS_SUBPLOT_OBJ, crawler.IS_LINKED_TO_ARRAY, crawler.DEPRECATED];
+
+crawler.crawl = function(attrs, callback, specifiedLevel) {


same here, I'd put this in coerce.js (if ok)

@etpinard same question here, how would it best fit, as a separate export or ... pls specify?

Similarly here:

export.crawl in coerce.js

etpinard · 2016-09-22T15:30:19Z

src/plot_api/plot_schema.js

@@ -56,16 +56,17 @@ PlotSchema.get = function() {
    return plotSchema;
 };

-PlotSchema.crawl = function(attrs, callback) {
+PlotSchema.crawl = function (attrs, callback, specifiedLevel) {
+    var level = specifiedLevel || 0;


very nicely done.

Is it possible to add a docstring on the usage? This feels very useful, but I don't immediately comprehend what it does.

Or maybe this isn't the method that needs the docstring, but from what I understand, these methods are critical for organizing the messy parts of working with the plot schema, so would be great to make preferred logic clear!

As I had to get familiar with it, I can add docs. Caveat, the nearly identical crawler was already in place (in plot_schema.js) and I'm not the biggest Plotly schema expert.

Thanks. Sorry for the extra burden. Not much necessary at all. Just a couple signposts to direct traffic. I just feel like we've been facing very similar challenges with these things, and I suspect a bug that I need to fix involves some of the same problems you've addressed here.

Definitely, at least any gaps in my understanding can be cleared :-) Sometimes I'm thinking of adding verbiage but end up being more concerned with the large lifecycle functions, some of which are a few hundred lines.

True, the crawler is already accessible publicly on Plotly.PlotSchema.crawl though it isn't an official plotly.js method yet.

Maybe we should add it the main Plotly object in v2.0.0 e.g. Plotly.crawlAttributes ?

etpinard · 2016-09-22T15:44:53Z

src/plots/plots.js

+            }
+        }
+
+        crawler.crawl(trace._module.attributes, callback);


Is the crawler able to catch nested attributes like error_y.array declared here ?

If not, it's not a big deal. I'm not a big fan of the _nestedAttributes pattern anyway. We could simple require nested attribute in the root attribute modules to make this work.

I don't know offhand, if it's not a big deal either way, I'll take a quick look and either cover it or reify it with a test case.

etpinard · 2016-09-22T15:47:28Z

src/transforms/groupby.js

    }

    return newData;
 }
+
+function getDeepProp(thing, propArray) {


getDeepProp and setDeepProp would be nice additions to src/lib/nested_property.js.

I'll take a look! Btw. I was also thinking about using deepExtendas an alternative to the attrib path arrays and setDeepProp but I had performance concerns, it's already in a deeply nested loop so we might want to iterate on performance at some point.

I'd vote for using setDeepProp - though I haven't ran any benchmarking.

Turns out that nested_property.js yields one single export, which is a function, do you want that I refactor its single usage (by coerce.js) such that nested_property.js can export multiple things?

etpinard · 2016-09-22T16:00:04Z

test/jasmine/tests/transform_groupby_test.js

+                expect(gd.data[0].x).toEqual([1, -1, -2, 0, 1, 2, 3]);
+                expect(gd.data[0].y).toEqual([1, 2, 3, 1, 2, 3, 1]);
+
+                expect(gd._fullData.length).toEqual(0); // fixme: it passes with 0; shouldn't it be 1? (one implied group)


Good point here.

What should a groupby transform with no groups field do?

In that case maybe it would be less surprising to have groupby not do anything (i.e. the identity transform).

I think it should also do the same as when transforms.groups.length === 0. Which leads to the question, what if transforms.groups.length < x.length and also, what we do now when arrays are of non-shared length.

What the code currently does is, it iterates through the elements of groups on 0 -> length - 1 i.e. the result will be empty.

While I can conditionalize it to do something differently (e.g. if(transforms.groups === void(0) || transforms.groups.length === 0) return; or some such) the user may pass e.g. one element in groups and gets another behavior.

@etpinard what's the desired outcome if

the user doesn't specify a transforms.groups

the user does specify a transforms.groups but it's null, [], undefined

the user specifies a transforms.groups array but its length is shorter than the length of the (shortest?) data array

I'm almost leaning toward leaving the code as it is (i.e. no added logic), in favor of letting others work with groupby as early as possible, but it's probably because I'm not yet familiar with strong conventions in this realm so I'll gladly do what's deemed best.

the user doesn't specify a transforms.groups

skip over the transform

the user does specify a transforms.groups but it's null, [], undefined

same here, skip over the transform

the user specifies a transforms.groups array but its length is shorter than the length of the (shortest?) data array

The resulting extended should as many points as items in groups. In other words, skip over trace items that can't be paired up with a groups item

etpinard · 2016-09-22T16:05:35Z

test/jasmine/tests/transform_groupby_test.js

+        });
+
+        // passes; looks OK
+        it('Plotly.plot should plot the transform traces', function(done) {


etpinard · 2016-09-22T16:07:15Z

test/jasmine/tests/transform_groupby_test.js

+                expect(gd.data[0].x).toEqual([1, -1, -2, 0, 1, 2, 3]);
+                expect(gd.data[0].y).toEqual([1, 2, 3, 1, 2, 3, 1]);
+
+                expect(gd._fullData.length).toEqual(1); // todo: confirm this result is OK


This result is ok.

Non-array transforms values should be ignored.

etpinard · 2016-09-22T16:07:47Z

test/jasmine/tests/transform_groupby_test.js

+                expect(gd.data[0].x).toEqual([1, -1, -2, 0, 1, 2, 3]);
+                expect(gd.data[0].y).toEqual([1, 2, 3, 1, 2, 3, 1]);
+
+                expect(gd._fullData.length).toEqual(1); // todo: confirm this result is OK


This is the expected result.

etpinard · 2016-09-22T16:12:49Z

test/jasmine/tests/transform_groupby_test.js

+                        }
+                    },
+                    b: {
+                        mode: 'markers+lines', // heterogeonos attributes are OK: group 'a' doesn't need to define this


etpinard · 2016-09-22T16:16:31Z

test/jasmine/tests/transform_groupby_test.js

+                line: {
+                    width: [4, 2, 4, 2, 2, 3, 3],
+                    // a general, not overridden array will be interpreted per group
+                    color: ['orange', 'red', 'green', 'cyan']


so what's the output here?

If I understand correctly, transforms[0].style[a_or_b].maker.color overrides the trace marker.color ?

Trace belonging to group a gets the marker.line.color from the split array: ['orange', 'red', 'cyan', 'pink']
Trace belonging to group b gets the marker.line.color from transforms.style.b.marker.line.color: yellow

I updated the test case to demonstrate it (will push soon).

Looks good. Thanks!

…ero length

etpinard · 2016-09-26T14:40:25Z

src/lib/coerce.js

+exports.UNDERSCORE_ATTRS = [exports.IS_SUBPLOT_OBJ, exports.IS_LINKED_TO_ARRAY, exports.DEPRECATED];
+
+/**
+ * Crawl the attribute tree, recursively calling a callback function


Thanks for 📚

etpinard · 2016-09-26T14:45:14Z

src/transforms/groupby.js

    var opts = state.transform;
-    var groups = opts.groups;
+    var groups = trace.transforms[state.transformIndex].groups;


what's the difference here between

state.transform // and trace.transforms[state.transformIndex]

??

@etpinard the latter is the pre-transform information. We want to obtain the groups. If a filter applies before a groupby then state.transform.groups will be an array that's already culled by the filter.

Thanks very much

etpinard · 2016-09-26T14:48:20Z

src/transforms/groupby.js

+    var arrayAttributes = attributeSet
+        .filter(function(array) {return Array.isArray(Lib.nestedProperty(trace, array).get());});
+
+    // fixme the O(n**3) complexity


True, we could probably do better in terms of performance, but O(n^3) where n is the number of attributes doesn't bother me too much,

I'll remove the comment and iterate on performance either separately or as needed (as you see fit)

Remove the comment for now. Like I said big loops over attributes don't bother me (as opposed to big loops over data arrays).

Thanks, done

etpinard

@monfera looks like all the functionality is now working. Nice work!

We'll merge this in after ⚡ing that obsolete comment.

etpinard · 2016-09-26T14:52:46Z

test/jasmine/tests/transform_groupby_test.js

+                expect(gd.data[0].x).toEqual([1, -1, -2, 0, 1, 2, 3]);
+                expect(gd.data[0].y).toEqual([1, 2, 3, 1, 2, 3, 1]);
+
+                expect(gd._fullData.length).toEqual(1); // fixme not: good, okay it's 1 here (one implied group / thing)


⚡ comment please

thanks, removed!

rreusser · 2016-09-26T15:51:34Z

src/plots/plots.js

+         *   Example of filled closure variable (expected to be initialized to []):
+         *        [["marker","size"],["marker","line","width"],["marker","line","color"]]
+         */
+        function callback(attr, attrName, attrs, level) {


Interesting! Thanks for the docs. This might be applicable in a few situations.

timelyportfolio and others added 11 commits September 14, 2016 09:56

move transforms to a "proper" place

9bb2b10

add in \ notin and within \ notwithin filter transforms; regi…

582d210

…ster `filter` with plotly

use value array for array comparisons and camelCase

d0ef359

accept '0' and ['0'] in filter

9574f4f

ignore character values for within filter

77e2b27

use Lib.isDateTime

aa1f1d3

clean and lint

6015768

remove filter register in validate_test

6daeac5

try to get header correct

3e0f209

minor cleanup

d735b64

commenting out tests not yet passing

a820b0c

monfera changed the title ~~Transforms [rebase of timelyportfolio's work]~~ [WIP] Transforms [rebase of timelyportfolio's work] Sep 14, 2016

require the transforms

2dd3646

etpinard added feature something new status: in progress labels Sep 14, 2016

etpinard added this to the v1.18.0 milestone Sep 14, 2016

rreusser reviewed Sep 14, 2016
View reviewed changes

etpinard reviewed Sep 15, 2016

View reviewed changes

monfera added 7 commits September 16, 2016 11:19

split large and growing test file to three

c30f94e

groupby generalization

e9bde33

(cherry picked from commit 7e967fe)

switch from 'styles' to 'style'

c6355e8

groupby generalization - update test cases

5765f66

adding ids to filtersrc

02742e8

[spacing][technical] just wrapping around test case in anticipation o…

b23d2ff

…f more cases inside

symmetry / degeneracy test cases (manually cherrypicked)

55f9e0c

etpinard reviewed Sep 22, 2016

View reviewed changes

etpinard mentioned this pull request Sep 23, 2016

(WIP) Filter and GroupBy Transforms #859

Closed

1 task

monfera added 7 commits September 23, 2016 17:24

PR feedback: comment and test case updates

1dd93bb

PR feedback: test with empty grouping information

1ea2fd7

PR feedback: moved isValObject into coerce

59b741d

PR feedback: moved crawler and its constituent parts into coerce

a5dad2a

Minor: var rename

79dcbc3

PR feedback: don't split if groups is unsupplied, non-array or of z…

aa70e4c

…ero length

PR feedback: disuse in favor of Lib.nestedProperty getter/setter

523f61b

etpinard reviewed Sep 26, 2016

View reviewed changes

etpinard added status: reviewable and removed status: in progress labels Sep 26, 2016

etpinard suggested changes Sep 26, 2016

View reviewed changes

rreusser reviewed Sep 26, 2016

View reviewed changes

PR feedback: pruning in-progress comments

4efe6a9

etpinard approved these changes Sep 26, 2016

View reviewed changes

etpinard merged commit fdeed44 into plotly:master Sep 26, 2016

This was referenced Sep 26, 2016

Filter and groupby transforms in main bundle #978

Merged

Transform input data: groupby, filter #917

Closed

[WIP] Transforms [rebase of timelyportfolio's work] #936

[WIP] Transforms [rebase of timelyportfolio's work] #936

Conversation

monfera commented Sep 14, 2016 • edited Loading

monfera commented Sep 14, 2016 • edited Loading

timelyportfolio commented Sep 14, 2016 • edited Loading

monfera commented Sep 14, 2016

monfera commented Sep 14, 2016 • edited Loading

etpinard commented Sep 14, 2016

rreusser Sep 14, 2016 • 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

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

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

etpinard Sep 22, 2016 • 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

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

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

monfera Sep 23, 2016 • 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

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

etpinard left a 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

monfera commented Sep 14, 2016 •

edited

Loading

monfera commented Sep 14, 2016 •

edited

Loading

timelyportfolio commented Sep 14, 2016 •

edited

Loading

monfera commented Sep 14, 2016 •

edited

Loading

rreusser Sep 14, 2016 •

edited

Loading

etpinard Sep 22, 2016 •

edited

Loading

monfera Sep 23, 2016 •

edited

Loading