Skip to content

Introducing transform plugins #499

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

Merged
merged 27 commits into from
Jul 22, 2016
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
53a5971
expose Lib on test dashboard window
etpinard May 3, 2016
9909382
lint
etpinard May 3, 2016
28147e2
add check so that we don't try to destroy gd if it DNE
etpinard May 3, 2016
9ac6d0f
make Plotly.register handle transform modules
etpinard May 3, 2016
06f2e6d
add support for transform plugins:
etpinard May 3, 2016
8ab9d0c
make Plotly.restyle support transforms:
etpinard May 3, 2016
2697975
add two example transform modules
etpinard May 3, 2016
56665f2
add transform test suite
etpinard May 3, 2016
aa274ef
update tests (plots.supplyDataDefaults -> supplyTraceDefaults)
etpinard May 3, 2016
a06441a
[tmp commit] add example transforms to ./lib/ Plotly for testing
etpinard May 3, 2016
0b62e45
Merge branch 'master' into transform-plugins
etpinard Jul 4, 2016
1c651e8
toimage_test: make sure that all graph divs are deleted
etpinard Jul 4, 2016
f93b9c1
plots: rm useless skip-over-step
etpinard Jul 4, 2016
ccbcb58
Merge branch 'master' into transform-plugins
etpinard Jul 18, 2016
37e291b
plots: make relinkPrivateKeys less strict
etpinard Jul 19, 2016
383c6d4
move common code in pushModule func
etpinard Jul 19, 2016
be36402
rm useless comment about fits ;)
etpinard Jul 19, 2016
0e1eb5a
do not add transforms container if fullTrace is no transform present
etpinard Jul 19, 2016
6a8e158
try more useless refs to input container in full trace
etpinard Jul 19, 2016
9a341c1
mutate uid of expanded traces w.r.t to expanded index:
etpinard Jul 19, 2016
1752d30
make uid of expanded trace valid alpha-numerical hashes
etpinard Jul 19, 2016
b5ec01e
add support for transform in plot schema and Plotly.validate
etpinard Jul 20, 2016
a6fda02
improve validate test cases
etpinard Jul 20, 2016
da2c24c
validate transform module on registration:
etpinard Jul 20, 2016
8d7dc46
improve transform tests
etpinard Jul 20, 2016
320d62e
Revert "[tmp commit] add example transforms to ./lib/ Plotly for test…
etpinard Jul 20, 2016
3e6e9c1
rm @assets path shortcut,
etpinard Jul 20, 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
2 changes: 2 additions & 0 deletions devtools/test_dashboard/devtools.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

var Fuse = require('fuse.js');
var mocks = require('../../build/test_dashboard_mocks.json');
var Lib = require('@src/lib');

// put d3 in window scope
var d3 = window.d3 = Plotly.d3;
Expand Down Expand Up @@ -119,6 +120,7 @@ var Tabs = {

// Bind things to the window
window.Tabs = Tabs;
window.Lib = Lib;
setInterval(function() {
window.gd = Tabs.getGraph() || Tabs.fresh();
window.fullLayout = window.gd._fullLayout;
Expand Down
5 changes: 4 additions & 1 deletion devtools/test_dashboard/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ var watchify = require('watchify');

var constants = require('../../tasks/util/constants');
var compress = require('../../tasks/util/compress_attributes');
var shortcutPaths = require('../../tasks/util/shortcut_paths');

var PORT = process.argv[2] || 3000;

Expand All @@ -32,7 +33,9 @@ b.on('update', bundlePlotly);

// Bundle devtools code
var devtoolsPath = path.join(constants.pathToRoot, 'devtools/test_dashboard');
var devtools = browserify(path.join(devtoolsPath, 'devtools.js'), {});
var devtools = browserify(path.join(devtoolsPath, 'devtools.js'), {
transform: [shortcutPaths]
});

var firstBundle = true;

Expand Down
6 changes: 6 additions & 0 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,10 @@ Core.register([
require('./scatterternary')
]);

// add filter plugin
Core.register([
require('../test/jasmine/assets/transforms/filter'),
require('../test/jasmine/assets/transforms/groupby')
]);

module.exports = Core;
24 changes: 13 additions & 11 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -854,9 +854,8 @@ Plotly.newPlot = function(gd, data, layout, config) {
function doCalcdata(gd) {
var axList = Plotly.Axes.list(gd),
fullData = gd._fullData,
fullLayout = gd._fullLayout;

var i, trace, module, cd;
fullLayout = gd._fullLayout,
i;

var calcdata = gd.calcdata = new Array(fullData.length);

Expand All @@ -882,12 +881,12 @@ function doCalcdata(gd) {
}

for(i = 0; i < fullData.length; i++) {
trace = fullData[i];
module = trace._module;
cd = [];
var trace = fullData[i],
_module = trace._module,
cd = [];

if(module && trace.visible === true) {
if(module.calc) cd = module.calc(gd, trace);
if(_module && trace.visible === true) {
if(_module.calc) cd = _module.calc(gd, trace);
}

// make sure there is a first point
Expand All @@ -901,7 +900,7 @@ function doCalcdata(gd) {
if(!cd[0].t) cd[0].t = {};
cd[0].trace = trace;

Lib.markTime('done with calcdata for '+i);
Lib.markTime('done with calcdata for ' + i);
calcdata[i] = cd;
}
}
Expand Down Expand Up @@ -1564,9 +1563,9 @@ Plotly.restyle = function restyle(gd, astr, val, traces) {

if(Object.keys(aobj).length) gd.changed = true;

if(isNumeric(traces)) traces=[traces];
if(isNumeric(traces)) traces = [traces];
else if(!Array.isArray(traces) || !traces.length) {
traces=gd._fullData.map(function(v,i) { return i; });
traces = gd.data.map(function(v, i) { return i; });
}

// recalcAttrs attributes need a full regeneration of calcdata
Expand Down Expand Up @@ -1734,6 +1733,9 @@ Plotly.restyle = function restyle(gd, astr, val, traces) {
continue;
}

// take no chances on transforms
if(ai.substr(0, 10) === 'transforms') docalc = true;

// set attribute in gd.data
undoit[ai] = a0();
for(i=0; i<traces.length; i++) {
Expand Down
25 changes: 19 additions & 6 deletions src/plotly.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,27 @@ exports.register = function register(_modules) {
for(var i = 0; i < _modules.length; i++) {
var newModule = _modules[i];

if(newModule && newModule.moduleType !== 'trace') {
if(!newModule) {
throw new Error('Invalid module was attempted to be registered!');
} else {
Plots.register(newModule, newModule.name, newModule.categories, newModule.meta);
}

switch(newModule.moduleType) {
case 'trace':
Plots.register(newModule, newModule.name, newModule.categories, newModule.meta);

if(!Plots.subplotsRegistry[newModule.basePlotModule.name]) {
Plots.registerSubplot(newModule.basePlotModule);
}

break;

case 'transform':
Plots.transformsRegistry[newModule.name] = newModule;

break;

if(!Plots.subplotsRegistry[newModule.basePlotModule.name]) {
Plots.registerSubplot(newModule.basePlotModule);
}
default:
throw new Error('Invalid module was attempted to be registered!');
}
}
};
Expand Down
119 changes: 97 additions & 22 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ var plots = module.exports = {};
var modules = plots.modules = {},
allTypes = plots.allTypes = [],
allCategories = plots.allCategories = {},
subplotsRegistry = plots.subplotsRegistry = {};
subplotsRegistry = plots.subplotsRegistry = {},
transformsRegistry = plots.transformsRegistry = {};

plots.attributes = require('./attributes');
plots.attributes.type.values = allTypes;
Expand Down Expand Up @@ -449,8 +450,7 @@ plots.supplyDefaults = function(gd) {
newData = gd.data || [],
modules = gd._modules = [];

var i, trace, fullTrace, _module, axList, ax;

var i, trace, _module, axList, ax;

// first fill in what we can of layout without looking at data
// because fullData needs a few things from layout
Expand All @@ -460,24 +460,7 @@ plots.supplyDefaults = function(gd) {
newFullLayout._dataLength = newData.length;

// then do the data
for(i = 0; i < newData.length; i++) {
trace = newData[i];

fullTrace = plots.supplyDataDefaults(trace, i, newFullLayout);
newFullData.push(fullTrace);

// detect plot type
if(plots.traceIs(fullTrace, 'cartesian')) newFullLayout._hasCartesian = true;
else if(plots.traceIs(fullTrace, 'gl3d')) newFullLayout._hasGL3D = true;
else if(plots.traceIs(fullTrace, 'geo')) newFullLayout._hasGeo = true;
else if(plots.traceIs(fullTrace, 'pie')) newFullLayout._hasPie = true;
else if(plots.traceIs(fullTrace, 'gl2d')) newFullLayout._hasGL2D = true;
else if(plots.traceIs(fullTrace, 'ternary')) newFullLayout._hasTernary = true;
else if('r' in fullTrace) newFullLayout._hasPolar = true;

_module = fullTrace._module;
if(_module && modules.indexOf(_module)===-1) modules.push(_module);
}
plots.supplyDataDefaults(newData, newFullData, newFullLayout, modules);

// special cases that introduce interactions between traces
for(i = 0; i < modules.length; i++) {
Expand Down Expand Up @@ -609,7 +592,61 @@ function relinkPrivateKeys(toLayout, fromLayout) {
}
}

plots.supplyDataDefaults = function(traceIn, i, layout) {
plots.supplyDataDefaults = function(dataIn, dataOut, layout, modules) {
var cnt = 0;

// push to array if item isn't already in array
function fill(arr, item) {
if(item && arr.indexOf(item) === -1) return arr.push(item);
}

// detect plot type
function detect(trace) {
if(plots.traceIs(trace, 'cartesian')) layout._hasCartesian = true;
else if(plots.traceIs(trace, 'gl3d')) layout._hasGL3D = true;
else if(plots.traceIs(trace, 'geo')) layout._hasGeo = true;
else if(plots.traceIs(trace, 'pie')) layout._hasPie = true;
else if(plots.traceIs(trace, 'gl2d')) layout._hasGL2D = true;
else if(plots.traceIs(trace, 'ternary')) layout._hasTernary = true;
else if('r' in trace) layout._hasPolar = true;
}

for(var i = 0; i < dataIn.length; i++) {
var trace = dataIn[i];

var fullTrace = plots.supplyTraceDefaults(trace, cnt, layout);

// keep track of pre-transform _input
var traceIn = fullTrace._input;
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't this just trace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Thanks for checking.


if(fullTrace.transforms && fullTrace.transforms.length) {
var expandedTraces = applyTransforms(fullTrace, dataOut, layout);

for(var j = 0; j < expandedTraces.length; j++) {
var expandedTrace = expandedTraces[j];
var fullExpandedTrace = plots.supplyTraceDefaults(expandedTrace, cnt, layout);

// copy refs
fullExpandedTrace._input = traceIn;
fullExpandedTrace._fullTransforms = fullTrace.transforms;
fullExpandedTrace._index = i;
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure what we need fullTrace.index for - as provided below in supplyTraceDefaults it's cnt, but we should give these better names than index and _index - maybe _inputIndex and _expandedIndex? I can imagine needing both in different cases...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what we need fullTrace.index

We need it to set the curveNumber event data field (see here).It is also used in the legend and selection code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but we should give these better names than index and _index - maybe _inputIndex and _expandedIndex

Good call. I like the sound of _expandedIndex @chriddyp you're ok with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If ok, I would not rename _index.


dataOut.push(fullExpandedTrace);
fill(modules, fullExpandedTrace._module);
detect(fullExpandedTrace);
cnt++;
}
}
else {
dataOut.push(fullTrace);
fill(modules, fullTrace._module);
detect(trace);
Copy link
Collaborator

Choose a reason for hiding this comment

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

detect(fullTrace); ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

in fact, this untransformed case needs i linked in as well. Might as well make eg:

function addFinalTrace(finalTrace) {
    finalTrace._input = trace;
    finalTrace._inputIndex = i;
    finalTrace._expandedIndex = cnt;
    dataOut.push(finalTrace);
    fill(modules, finalTrace);
    detect(finalTrace);
    cnt++;
}

since all of that needs to happen identically whichever way you added a trace to _fullData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch ⚾ . Thanks!

cnt++;
}
}
};

plots.supplyTraceDefaults = function(traceIn, i, layout) {
var traceOut = {},
defaultColor = Color.defaults[i % Color.defaults.length];

Expand Down Expand Up @@ -666,6 +703,8 @@ plots.supplyDataDefaults = function(traceIn, i, layout) {
coerce('showlegend');
coerce('legendgroup');
}

supplyTransformDefaults(traceIn, traceOut, layout);
}

// NOTE: I didn't include fit info at all... for now I think it can stay
Expand All @@ -678,6 +717,42 @@ plots.supplyDataDefaults = function(traceIn, i, layout) {
return traceOut;
};

function supplyTransformDefaults(traceIn, traceOut, layout) {
var containerIn = traceIn.transforms || [],
containerOut = traceOut.transforms = [];
Copy link
Member

Choose a reason for hiding this comment

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

I think this line here ensures that every fullTrace will now have a transforms array? Most of the time it will be empty. Some tests that have nothing to do with transforms are failing due to an empty transforms array appearing. I think this is the culprit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I can remove it.

Copy link
Member

Choose a reason for hiding this comment

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

just verified.
putting a

    if(!Array.isArray(traceIn.transforms)) {
        return;
    }

at the top of this function remedies the issue. Though perhaps this breaks other things. Not sure.


for(var i = 0; i < containerIn.length; i++) {
var transformIn = containerIn[i],
type = transformIn.type,
_module = transformsRegistry[type];

var transformOut = _module.supplyDefaults(transformIn, traceOut, layout);
transformOut.type = type;

containerOut.push(transformOut);
}
}

function applyTransforms(fullTrace, fullData, layout) {
var container = fullTrace.transforms,
dataOut = [fullTrace];

for(var i = 0; i < container.length; i++) {
var transform = container[i],
type = transform.type,
_module = transformsRegistry[type];

dataOut = _module.transform(dataOut, {
transform: transform,
fullTrace: fullTrace,
fullData: fullData,
layout: layout
});
}

return dataOut;
}

plots.supplyLayoutGlobalDefaults = function(layoutIn, layoutOut) {
function coerce(attr, dflt) {
return Lib.coerce(layoutIn, layoutOut, plots.layoutAttributes, attr, dflt);
Expand Down
1 change: 1 addition & 0 deletions tasks/util/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ module.exports = {

pathToJasmineTests: path.join(pathToRoot, 'test/jasmine/tests'),
pathToJasmineBundleTests: path.join(pathToRoot, 'test/jasmine/bundle_tests'),
pathToJasmineTestAssets: path.join(pathToRoot, 'test/jasmine/assets'),

uglifyOptions: {
fromString: true,
Expand Down
3 changes: 2 additions & 1 deletion tasks/util/shortcut_paths.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ var constants = require('./constants');
var shortcutsConfig = {
'@src': constants.pathToSrc,
'@lib': constants.pathToLib,
'@mocks': constants.pathToTestImageMocks
'@mocks': constants.pathToTestImageMocks,
'@assets': constants.pathToJasmineTestAssets
};

module.exports = transformTools.makeRequireTransform('requireTransform',
Expand Down
3 changes: 2 additions & 1 deletion test/jasmine/assets/destroy_graph_div.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@

module.exports = function destroyGraphDiv() {
var gd = document.getElementById('graph');
document.body.removeChild(gd);

if(gd) document.body.removeChild(gd);
};
Loading