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 25 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;
1 change: 1 addition & 0 deletions src/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ var lib = module.exports = {};

lib.nestedProperty = require('./nested_property');
lib.isPlainObject = require('./is_plain_object');
lib.isValObject = require('./is_val_object');
lib.isArray = require('./is_array');

var coerceModule = require('./coerce');
Expand Down
15 changes: 15 additions & 0 deletions src/lib/is_val_object.js
Original file line number Diff line number Diff line change
@@ -0,0 +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';

// returns true for a valid value object and false for tree nodes in the attribute hierarchy
module.exports = function(obj) {
return obj && obj.valType !== undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice job, looking over circular dependencies.

I think this function would make more sense in the src/lib/coerce.js module. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, coerce.js looks like the best home for it.

Copy link
Contributor Author

@monfera monfera Sep 23, 2016

Choose a reason for hiding this comment

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

@etpinard Where do you specifically want to see it in coerce.js? Totally separate export (exports.isValObject), or as an element inside exports.valObject, e.g. object: {...} in which case the actual function is in coerceFunction, or something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be exports.isValObject in coerce.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 moved it to coerce.js and similarly to how other coerce constituents are handled, I directly reexport as Lib.isValObject. If it's desirable, I can phase out PlotSchema.isValObject in favor of Lib.isValObject, what do you think?

Copy link
Contributor

@etpinard etpinard Sep 23, 2016

Choose a reason for hiding this comment

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

If it's desirable, I can phase out PlotSchema.isValObject in favor of Lib.isValObject, what do you think?

Not yet as Plotly.PlotSchema.isValObject is currently exposed publicly.

};
35 changes: 35 additions & 0 deletions src/plot_api/crawler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* 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('../lib');

var crawler = module.exports = {};

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

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly here:

export.crawl in coerce.js

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

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

callback(attr, attrName, attrs, level);

if(Lib.isValObject(attr)) return;
if(Lib.isPlainObject(attr)) crawler.crawl(attr, callback, level + 1);
});
};
44 changes: 13 additions & 31 deletions src/plot_api/plot_schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ var Plotly = require('../plotly');
var Registry = require('../registry');
var Plots = require('../plots/plots');
var Lib = require('../lib');
var crawler = require('./crawler');

// FIXME polar attribute are not part of Plotly yet
var polarAreaAttrs = require('../plots/polar/area_attributes');
Expand All @@ -23,13 +24,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 +51,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 = crawler.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 +113,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 +140,7 @@ function getTransformAttributes(name) {
function getDefs() {
plotSchema.defs = {
valObjects: Lib.valObjects,
metaKeys: UNDERSCORE_ATTRS.concat(['description', 'role'])
metaKeys: crawler.UNDERSCORE_ATTRS.concat(['description', 'role'])
};
}

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

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

// helper methods
Expand All @@ -267,7 +249,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];
crawler.UNDERSCORE_ATTRS.indexOf(k) === -1) delete attributes[k];
});
return attributes;
}
Expand Down Expand Up @@ -321,7 +303,7 @@ function handleSubplotObjs(layoutAttributes) {
isSubplotObj = subplotRegistry.attrRegex.test(k);
}

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

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

function callback(attr, attrName, attrs) {
if(attr[IS_LINKED_TO_ARRAY] !== true) return;
if(attr[crawler.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[crawler.IS_LINKED_TO_ARRAY];

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

PlotSchema.crawl(layoutAttributes, callback);
crawler.crawl(layoutAttributes, callback);
}
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 crawler = require('./../plot_api/crawler');
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());
}
}

crawler.crawl(trace._module.attributes, callback);
Copy link
Contributor

@etpinard etpinard Sep 22, 2016

Choose a reason for hiding this comment

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

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.

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 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.


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
Loading