Skip to content
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.

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

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

Merged
merged 34 commits into from
Sep 26, 2016
Merged
Show file tree
Hide file tree
Changes from 24 commits
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
9 changes: 9 additions & 0 deletions lib/filter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/**
* Copyright 2012-2016, Plotly, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

module.exports = require('../src/transforms/filter');
5 changes: 5 additions & 0 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,9 @@ Plotly.register([
require('./scattermapbox')
]);

// add transforms
Plotly.register([
require('./filter')
]);

module.exports = Plotly;
7 changes: 4 additions & 3 deletions src/plot_api/plot_schema.js
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
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
Original file line number Diff line number Diff line change
@@ -1,11 +1,20 @@
/**
* Copyright 2012-2016, Plotly, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

'use strict';

var isNumeric = require('fast-isnumeric');

// var Lib = require('@src/lib');
var Lib = require('../../../../src/lib');
var Lib = require('../lib');

/* eslint no-unused-vars: 0*/


// so that Plotly.register knows what to do with it
exports.moduleType = 'transform';

Expand All @@ -16,17 +25,21 @@ exports.name = 'filter';
exports.attributes = {
operation: {
valType: 'enumerated',
values: ['=', '<', '>'],
values: ['=', '<', '>', 'within', 'notwithin', 'in', 'notin'],
dflt: '='
},
value: {
valType: 'number',
valType: 'any',
dflt: 0
},
filtersrc: {
Copy link
Contributor

@rreusser rreusser Sep 14, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

you mean ids ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

valType: 'enumerated',
values: ['x', 'y'],
dflt: 'x'
dflt: 'x',
ids: {
valType: 'data_array',
description: 'A list of keys for object constancy of data points during animation'
}
}
};

Expand Down Expand Up @@ -54,6 +67,16 @@ exports.supplyDefaults = function(transformIn, fullData, layout) {
coerce('value');
coerce('filtersrc');

// numeric values as character should be converted to numeric
if(Array.isArray(transformOut.value)) {
transformOut.value = transformOut.value.map(function(v) {
if(isNumeric(v)) v = +v;
return v;
});
} else {
if(isNumeric(transformOut.value)) transformOut.value = +transformOut.value;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

// or some more complex logic using fullData and layout

return transformOut;
Expand Down Expand Up @@ -121,6 +144,16 @@ function transformOne(trace, state) {

function getFilterFunc(opts) {
var value = opts.value;
// if value is not array then coerce to
// an array of [value,value] so the
// filter function will work
// but perhaps should just error out
var valueArr = [];
if(!Array.isArray(value)) {
valueArr = [value, value];
} else {
valueArr = value;
}

switch(opts.operation) {
case '=':
Expand All @@ -129,6 +162,30 @@ function getFilterFunc(opts) {
return function(v) { return v < value; };
case '>':
return function(v) { return v > value; };
case 'within':
return function(v) {
// if character then ignore with no side effect
function notDateNumber(d) {
return !(isNumeric(d) || Lib.isDateTime(d));
}
if(valueArr.some(notDateNumber)) {
return true;
}

// keep the = ?
return v >= Math.min.apply(null, valueArr) &&
v <= Math.max.apply(null, valueArr);
};
case 'notwithin':
return function(v) {
// keep the = ?
return !(v >= Math.min.apply(null, valueArr) &&
v <= Math.max.apply(null, valueArr));
};
case 'in':
return function(v) { return valueArr.indexOf(v) >= 0; };
case 'notin':
return function(v) { return valueArr.indexOf(v) === -1; };
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
/**
* Copyright 2012-2016, Plotly, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

'use strict';

// var Lib = require('@src/lib');
var Lib = require('../../../../src/lib');
var Lib = require('../lib');

/* eslint no-unused-vars: 0*/

Expand All @@ -22,7 +30,7 @@ exports.attributes = {
valType: 'data_array',
dflt: []
},
groupColors: {
style: {
valType: 'any',
dflt: {}
}
Expand Down Expand Up @@ -53,7 +61,7 @@ exports.supplyDefaults = function(transformIn, fullData, layout) {
if(!active) return transformOut;

coerce('groups');
coerce('groupColors');
coerce('style');

// or some more complex logic using fullData and layout

Expand Down Expand Up @@ -82,14 +90,16 @@ exports.transform = function(data, state) {

var newData = [];

data.forEach(function(trace) {
newData = newData.concat(transformOne(trace, state));
data.forEach(function(trace, i) {

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

return newData;
};

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

var opts = state.transform;
var groups = opts.groups;

Expand All @@ -98,28 +108,67 @@ function transformOne(trace, state) {
});

var newData = new Array(groupNames.length);
var len = Math.min(trace.x.length, trace.y.length, groups.length);
var len = groups.length;

var style = opts.style || {};

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

var initializeArray = function(newTrace, a) {
setDeepProp(newTrace, a, []);
};

var pasteArray = function(newTrace, trace, j, a) {
getDeepProp(newTrace, a).push(getDeepProp(trace, a)[j]);
};

// fixme the O(n**3) complexity
Copy link
Contributor

Choose a reason for hiding this comment

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

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,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done

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

// TODO is this the best pattern ???
// maybe we could abstract this out
var newTrace = newData[i] = Lib.extendDeep({}, trace);

newTrace.x = [];
newTrace.y = [];
topLevelAttributes.forEach(initializeArray.bind(null, newTrace));

for(var j = 0; j < len; j++) {
if(groups[j] !== groupName) continue;

newTrace.x.push(trace.x[j]);
newTrace.y.push(trace.y[j]);
topLevelAttributes.forEach(pasteArray.bind(0, newTrace, trace, j));
}

newTrace.name = groupName;
newTrace.marker.color = opts.groupColors[groupName];

// there's no need to coerce style[groupName] here
// as another round of supplyDefaults is done on the transformed traces
newTrace = Lib.extendDeep(newTrace, style[groupName] || {});
Copy link
Contributor

Choose a reason for hiding this comment

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

very nice.

Copy link
Contributor

Choose a reason for hiding this comment

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

... but maybe add a comment mentioning there's no need to coerce style[groupName] here as another round of supplyDefaults is done on the transformed traces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had some false starts but as it turned out, almost all code was already present :-)

}

return newData;
}

function getDeepProp(thing, propArray) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

var result = thing;
var i;
for(i = 0; i < propArray.length; i++) {
result = result[propArray[i]];
if(result === void(0)) {
return result;
}
}
return result;
}

function setDeepProp(thing, propArray, value) {
var current = thing;
var i;
for(i = 0; i < propArray.length - 1; i++) {
if(current[propArray[i]] === void(0)) {
current[propArray[i]] = {};
}
current = current[propArray[i]];
}
current[propArray[propArray.length - 1]] = value;
}
18 changes: 18 additions & 0 deletions test/jasmine/assets/assert_dims.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';

var d3 = require('d3');

module.exports = function assertDims(dims) {
var traces = d3.selectAll('.trace');

expect(traces.size())
.toEqual(dims.length, 'to have correct number of traces');

traces.each(function(_, i) {
var trace = d3.select(this);
var points = trace.selectAll('.point');

expect(points.size())
.toEqual(dims[i], 'to have correct number of pts in trace ' + i);
});
};
33 changes: 33 additions & 0 deletions test/jasmine/assets/assert_style.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
'use strict';

var d3 = require('d3');

module.exports = function assertStyle(dims, color, opacity) {
var N = dims.reduce(function(a, b) {
return a + b;
});

var traces = d3.selectAll('.trace');
expect(traces.size())
.toEqual(dims.length, 'to have correct number of traces');

expect(d3.selectAll('.point').size())
.toEqual(N, 'to have correct total number of points');

traces.each(function(_, i) {
var trace = d3.select(this);
var points = trace.selectAll('.point');

expect(points.size())
.toEqual(dims[i], 'to have correct number of pts in trace ' + i);

points.each(function() {
var point = d3.select(this);

expect(point.style('fill'))
.toEqual(color[i], 'to have correct pt color');
expect(+point.style('opacity'))
.toEqual(opacity[i], 'to have correct pt opacity');
});
});
};
4 changes: 2 additions & 2 deletions test/jasmine/tests/plotschema_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ var Plotly = require('@lib/index');
var Lib = require('@src/lib');

Plotly.register([
// until they become official
require('../assets/transforms/filter')
require('@src/transforms/filter'),
require('@src/transforms/groupby')
]);

describe('plot schema', function() {
Expand Down
Loading