Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 34 commits into from
Sep 26, 2016
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
9bb2b10
move `transforms` to a "proper" place
timelyportfolio Aug 3, 2016
582d210
add `in` \ `notin` and `within` \ `notwithin` filter transforms; regi…
timelyportfolio Aug 3, 2016
d0ef359
use value array for array comparisons and camelCase
timelyportfolio Aug 4, 2016
9574f4f
accept '0' and ['0'] in filter
timelyportfolio Aug 12, 2016
77e2b27
ignore character values for `within` filter
timelyportfolio Aug 12, 2016
aa1f1d3
use `Lib.isDateTime`
timelyportfolio Aug 12, 2016
6015768
clean and lint
timelyportfolio Aug 17, 2016
6daeac5
remove filter register in validate_test
timelyportfolio Aug 17, 2016
3e0f209
try to get header correct
timelyportfolio Aug 17, 2016
d735b64
minor cleanup
monfera Sep 14, 2016
a820b0c
commenting out tests not yet passing
monfera Sep 14, 2016
2dd3646
require the transforms
monfera Sep 14, 2016
c30f94e
split large and growing test file to three
monfera Sep 16, 2016
e9bde33
groupby generalization
monfera Sep 16, 2016
c6355e8
switch from 'styles' to 'style'
monfera Sep 16, 2016
5765f66
groupby generalization - update test cases
monfera Sep 16, 2016
02742e8
adding ids to filtersrc
monfera Sep 16, 2016
b23d2ff
[spacing][technical] just wrapping around test case in anticipation o…
monfera Sep 16, 2016
55f9e0c
symmetry / degeneracy test cases (manually cherrypicked)
monfera Sep 16, 2016
462b2f0
test heterogeneous attributes, deep nesting and overridden attributes
monfera Sep 16, 2016
9cafe1e
groupby: don't mandate styling for each group, or any style at all
monfera Sep 16, 2016
87de31f
groupby test for no style present
monfera Sep 16, 2016
61f3385
hierarchical property split (squashed)
monfera Sep 21, 2016
57d0c13
reuse the plotly_schema crawler and fix some crawling issues (squashed)
monfera Sep 22, 2016
7085729
resolved circular dependency by extracting the crawler
monfera Sep 22, 2016
392844d
sync up filter and groupby; fix combined test case which used to pass…
monfera Sep 22, 2016
1dd93bb
PR feedback: comment and test case updates
monfera Sep 22, 2016
1ea2fd7
PR feedback: test with empty grouping information
monfera Sep 23, 2016
59b741d
PR feedback: moved isValObject into coerce
monfera Sep 23, 2016
a5dad2a
PR feedback: moved crawler and its constituent parts into coerce
monfera Sep 23, 2016
79dcbc3
Minor: var rename
monfera Sep 23, 2016
aa70e4c
PR feedback: don't split if `groups` is unsupplied, non-array or of z…
monfera Sep 23, 2016
523f61b
PR feedback: disuse in favor of Lib.nestedProperty getter/setter
monfera Sep 23, 2016
4efe6a9
PR feedback: pruning in-progress comments
monfera Sep 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/plot_api/plot_schema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,17 @@ PlotSchema.get = function() {
return plotSchema;
};

PlotSchema.crawl = function(attrs, callback) {
PlotSchema.crawl = function (attrs, callback, specifiedLevel) {
var level = specifiedLevel || 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

very nicely done.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Object.keys(attrs).forEach(function(attrName) {
var attr = attrs[attrName];

if(UNDERSCORE_ATTRS.indexOf(attrName) !== -1) return;

callback(attr, attrName, attrs);
callback(attr, attrName, attrs, level);

if(PlotSchema.isValObject(attr)) return;
if(Lib.isPlainObject(attr)) PlotSchema.crawl(attr, callback);
if(Lib.isPlainObject(attr)) PlotSchema.crawl(attr, callback, level + 1);
});
};

Expand Down
23 changes: 23 additions & 0 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

'use strict';

var PlotSchema = require('./../plot_api/plot_schema')
var d3 = require('d3');
var isNumeric = require('fast-isnumeric');

Expand Down Expand Up @@ -786,6 +787,27 @@ function applyTransforms(fullTrace, fullData, layout) {
var container = fullTrace.transforms,
dataOut = [fullTrace];

var attributeSets = dataOut.map(function(trace) {

var arraySplitAttributes = [];

var stack = [];

function callback(attr, attrName, attrs, level) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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


stack = stack.slice(0, level).concat([attrName]);

var splittableAttr = attr.valType === 'data_array' || attr.arrayOk === true
if(splittableAttr) {
arraySplitAttributes.push(stack.slice());
}
}

PlotSchema.crawl(trace._module.attributes, callback);

return arraySplitAttributes;
});

for(var i = 0; i < container.length; i++) {
var transform = container[i],
type = transform.type,
Expand All @@ -796,6 +818,7 @@ function applyTransforms(fullTrace, fullData, layout) {
transform: transform,
fullTrace: fullTrace,
fullData: fullData,
attributeSets: attributeSets,
layout: layout
});
}
Expand Down
46 changes: 4 additions & 42 deletions src/transforms/groupby.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,20 +90,15 @@ exports.transform = function(data, state) {

var newData = [];

data.forEach(function(trace) {
data.forEach(function(trace, i) {

var splittingAttributes = [];

var attributes = trace._module.attributes;
crawl(attributes, splittingAttributes);

newData = newData.concat(transformOne(trace, state, splittingAttributes));
newData = newData.concat(transformOne(trace, state, state.attributeSets[i]));
});

return newData;
};

function transformOne(trace, state, splittingAttributes) {
function transformOne(trace, state, attributeSet) {

var opts = state.transform;
var groups = opts.groups;
Expand All @@ -117,7 +112,7 @@ function transformOne(trace, state, splittingAttributes) {

var style = opts.style || {};

var topLevelAttributes = splittingAttributes
var topLevelAttributes = attributeSet
.filter(function(array) {return Array.isArray(getDeepProp(trace, array));});

var initializeArray = function(newTrace, a) {
Expand Down Expand Up @@ -177,36 +172,3 @@ function setDeepProp(thing, propArray, value) {
}
current[propArray[propArray.length - 1]] = value;
}

// fixme check if similar functions in plot_schema.js can be reused
function crawl(attrs, list, path) {
path = path || [];

Object.keys(attrs).forEach(function(attrName) {
var attr = attrs[attrName];
var _path = path.slice();
_path.push(attrName);

if(attrName.charAt(0) === '_') return;

callback(attr, list, _path);

if(isValObject(attr)) return;
if(isPlainObject(attr)) crawl(attr, list, _path);
});
}

function isValObject(obj) {
return obj && obj.valType !== undefined;
}

function callback(attr, list, path) {
// see schema.defs for complete list of 'val types'
if(attr.valType === 'data_array' || attr.arrayOk === true) {
list.push(path);
}
}

function isPlainObject(obj) {
return Object.prototype.toString.call(obj) === '[object Object]';
}