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 all 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;
54 changes: 54 additions & 0 deletions src/lib/coerce.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,17 @@
var isNumeric = require('fast-isnumeric');
var tinycolor = require('tinycolor2');
var nestedProperty = require('./nested_property');
var isPlainObject = require('./is_plain_object');

var getColorscale = require('../components/colorscale/get_scale');
var colorscaleNames = Object.keys(require('../components/colorscale/scales'));

var idRegex = /^([2-9]|[1-9][0-9]+)$/;

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

exports.valObjects = {
data_array: {
// You can use *dflt=[] to force said array to exist though.
Expand Down Expand Up @@ -355,3 +360,52 @@ exports.validate = function(value, opts) {
valObject.coerceFunction(value, propMock, failed, opts);
return out !== failed;
};

/*
* returns true for a valid value object and false for tree nodes in the attribute hierarchy
*/
exports.isValObject = isValObject;

exports.IS_SUBPLOT_OBJ = '_isSubplotObj';
exports.IS_LINKED_TO_ARRAY = '_isLinkedToArray';
exports.DEPRECATED = '_deprecated';

// list of underscore attributes to keep in schema as is
exports.UNDERSCORE_ATTRS = [exports.IS_SUBPLOT_OBJ, exports.IS_LINKED_TO_ARRAY, exports.DEPRECATED];

/**
* Crawl the attribute tree, recursively calling a callback function
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for 📚

*
* @param {object} attrs
* The node of the attribute tree (e.g. the root) from which recursion originates
* @param {Function} callback
* A callback function with the signature:
* @callback callback
* @param {object} attr an attribute
* @param {String} attrName name string
* @param {object[]} attrs all the attributes
* @param {Number} level the recursion level, 0 at the root
* @param {Number} [specifiedLevel]
* The level in the tree, in order to let the callback function detect descend or backtrack,
* typically unsupplied (implied 0), just used by the self-recursive call.
* The necessity arises because the tree traversal is not controlled by callback return values.
* The decision to not use callback return values for controlling tree pruning arose from
* the goal of keeping the crawler backwards compatible. Observe that one of the pruning conditions
* precedes the callback call.
*
* @return {object} transformOut
* copy of transformIn that contains attribute defaults
*/
exports.crawl = function(attrs, callback, specifiedLevel) {
var level = specifiedLevel || 0;
Object.keys(attrs).forEach(function(attrName) {
var attr = attrs[attrName];

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

callback(attr, attrName, attrs, level);

if(isValObject(attr)) return;
if(isPlainObject(attr)) exports.crawl(attr, callback, level + 1);
});
};
6 changes: 6 additions & 0 deletions src/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ lib.coerce = coerceModule.coerce;
lib.coerce2 = coerceModule.coerce2;
lib.coerceFont = coerceModule.coerceFont;
lib.validate = coerceModule.validate;
lib.isValObject = coerceModule.isValObject;
lib.crawl = coerceModule.crawl;
lib.IS_SUBPLOT_OBJ = coerceModule.IS_SUBPLOT_OBJ;
lib.IS_LINKED_TO_ARRAY = coerceModule.IS_LINKED_TO_ARRAY;
lib.DEPRECATED = coerceModule.DEPRECATED;
lib.UNDERSCORE_ATTRS = coerceModule.UNDERSCORE_ATTRS;

var datesModule = require('./dates');
lib.dateTime2ms = datesModule.dateTime2ms;
Expand Down
43 changes: 12 additions & 31 deletions src/plot_api/plot_schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,7 @@ var extendDeep = Lib.extendDeep;
var extendDeepAll = Lib.extendDeepAll;

var NESTED_MODULE = '_nestedModules',
COMPOSED_MODULE = '_composedModules',
IS_SUBPLOT_OBJ = '_isSubplotObj',
IS_LINKED_TO_ARRAY = '_isLinkedToArray',
DEPRECATED = '_deprecated';

// list of underscore attributes to keep in schema as is
var UNDERSCORE_ATTRS = [IS_SUBPLOT_OBJ, IS_LINKED_TO_ARRAY, DEPRECATED];
COMPOSED_MODULE = '_composedModules';

var plotSchema = {
traces: {},
Expand All @@ -56,22 +50,9 @@ PlotSchema.get = function() {
return plotSchema;
};

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

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

callback(attr, attrName, attrs);

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

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

function getTraceAttributes(type) {
var globalAttributes = Plots.attributes,
Expand Down Expand Up @@ -131,13 +112,13 @@ function getLayoutAttributes() {
// FIXME polar layout attributes
layoutAttributes = assignPolarLayoutAttrs(layoutAttributes);

// add IS_SUBPLOT_OBJ attribute
// add crawler.IS_SUBPLOT_OBJ attribute
layoutAttributes = handleSubplotObjs(layoutAttributes);

layoutAttributes = removeUnderscoreAttrs(layoutAttributes);
mergeValTypeAndRole(layoutAttributes);

// generate IS_LINKED_TO_ARRAY structure
// generate crawler.IS_LINKED_TO_ARRAY structure
handleLinkedToArray(layoutAttributes);

plotSchema.layout = { layoutAttributes: layoutAttributes };
Expand All @@ -158,7 +139,7 @@ function getTransformAttributes(name) {
function getDefs() {
plotSchema.defs = {
valObjects: Lib.valObjects,
metaKeys: UNDERSCORE_ATTRS.concat(['description', 'role'])
metaKeys: Lib.UNDERSCORE_ATTRS.concat(['description', 'role'])
};
}

Expand Down Expand Up @@ -241,7 +222,7 @@ function mergeValTypeAndRole(attrs) {
}
}

PlotSchema.crawl(attrs, callback);
Lib.crawl(attrs, callback);
}

// helper methods
Expand All @@ -267,7 +248,7 @@ function getModule(arg) {
function removeUnderscoreAttrs(attributes) {
Object.keys(attributes).forEach(function(k) {
if(k.charAt(0) === '_' &&
UNDERSCORE_ATTRS.indexOf(k) === -1) delete attributes[k];
Lib.UNDERSCORE_ATTRS.indexOf(k) === -1) delete attributes[k];
});
return attributes;
}
Expand Down Expand Up @@ -321,7 +302,7 @@ function handleSubplotObjs(layoutAttributes) {
isSubplotObj = subplotRegistry.attrRegex.test(k);
}

if(isSubplotObj) layoutAttributes[k][IS_SUBPLOT_OBJ] = true;
if(isSubplotObj) layoutAttributes[k][Lib.IS_SUBPLOT_OBJ] = true;
});
});

Expand All @@ -331,17 +312,17 @@ function handleSubplotObjs(layoutAttributes) {
function handleLinkedToArray(layoutAttributes) {

function callback(attr, attrName, attrs) {
if(attr[IS_LINKED_TO_ARRAY] !== true) return;
if(attr[Lib.IS_LINKED_TO_ARRAY] !== true) return;

// TODO more robust logic
var itemName = attrName.substr(0, attrName.length - 1);

delete attr[IS_LINKED_TO_ARRAY];
delete attr[Lib.IS_LINKED_TO_ARRAY];

attrs[attrName] = { items: {} };
attrs[attrName].items[itemName] = attr;
attrs[attrName].role = 'object';
}

PlotSchema.crawl(layoutAttributes, callback);
Lib.crawl(layoutAttributes, callback);
}
39 changes: 38 additions & 1 deletion src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,41 @@ function applyTransforms(fullTrace, fullData, layout) {
var container = fullTrace.transforms,
dataOut = [fullTrace];

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

var arraySplitAttributes = [];

var stack = [];

/**
* A closure that gathers attribute paths into its enclosed arraySplitAttributes
* Attribute paths are collected iff their leaf node is a splittable attribute
* @callback callback
* @param {object} attr an attribute
* @param {String} attrName name string
* @param {object[]} attrs all the attributes
* @param {Number} level the recursion level, 0 at the root
* @closureVariable {String[][]} arraySplitAttributes the set of gathered attributes
* Example of filled closure variable (expected to be initialized to []):
* [["marker","size"],["marker","line","width"],["marker","line","color"]]
*/
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());
}
}

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

return arraySplitAttributes.map(function(path) {
return path.join('.');
});
});

for(var i = 0; i < container.length; i++) {
var transform = container[i],
type = transform.type,
Expand All @@ -796,7 +831,9 @@ function applyTransforms(fullTrace, fullData, layout) {
transform: transform,
fullTrace: fullTrace,
fullData: fullData,
layout: layout
attributeSets: attributeSets,
layout: layout,
transformIndex: i
});
}
}
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'
values: ['x', 'y', 'ids'],
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
Loading