Skip to content

Transform react #2577

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 26 commits into from
Apr 26, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
bbe3533
remove no longer needed hack for finance in findArrayAttributes
alexcjohnson Apr 18, 2018
ed24765
_commonLength -> _length
alexcjohnson Apr 18, 2018
fcc459d
fix #2508, fix #2470 - problems with Plotly.react and aggregate trans…
alexcjohnson Apr 18, 2018
7044a13
standardize transforms handling of _length
alexcjohnson Apr 19, 2018
88b7b43
:racehorse: refactor sort transform from O(n^2) to O(n)
alexcjohnson Apr 19, 2018
cf4c9c3
heatmap&carpet/has_columns -> Lib.is1D
alexcjohnson Apr 20, 2018
e84d4b9
close #1410 - yes, stop pruning in nestedProperty
alexcjohnson Apr 20, 2018
b436d52
ensure every trace defines _length in supplyDefaults, and abort trans…
alexcjohnson Apr 22, 2018
2a41f9e
react+transforms PR review edits
alexcjohnson Apr 24, 2018
965bcfb
fixes and test for checklist in #2508
alexcjohnson Apr 24, 2018
6fae229
some fixes and tests for empty data arrays
alexcjohnson Apr 24, 2018
79295f1
rename violins mock that doesn't contain violin traces
alexcjohnson Apr 25, 2018
5e9aa65
transforms mock
alexcjohnson Apr 26, 2018
690eb95
clean up keyedContainer for possibly missing array
alexcjohnson Apr 26, 2018
a244cec
violin should not explicitly set whiskerwidth
alexcjohnson Apr 26, 2018
92bd5d2
add _length and stop slicing in scattercarpet
alexcjohnson Apr 26, 2018
dc6de2f
clean up handling of colorscale defaults
alexcjohnson Apr 26, 2018
03956e1
include count aggregates in _arrayAttrs - so they remap correctly
alexcjohnson Apr 26, 2018
f439e41
in diffData I had _fullInput in the new trace, but not the old!
alexcjohnson Apr 26, 2018
e47e6a9
_compareAsJSON for groupby styles
alexcjohnson Apr 26, 2018
aa30ad6
update plot_api_test to :lock: recent changes in react/transforms etc
alexcjohnson Apr 26, 2018
4c70826
lint
alexcjohnson Apr 26, 2018
7279b55
+shapes & annotations3d in react-noop test
alexcjohnson Apr 26, 2018
3e250df
tweak docs & remove commented-out code
alexcjohnson Apr 26, 2018
cd08479
reactWith -> reactTo
alexcjohnson Apr 26, 2018
0414147
separate svg and gl traces in react-noop tests
alexcjohnson Apr 26, 2018
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
5 changes: 5 additions & 0 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -1178,6 +1178,11 @@ plots.supplyTraceDefaults = function(traceIn, colorIndex, layout, traceInIndex)
};

plots.supplyTransformDefaults = function(traceIn, traceOut, layout) {
// For now we only allow transforms on 1D traces, ie those that specify a _length.
// If we were to implement 2D transforms, we'd need to have each transform
// describe its own applicability and disable itself when it doesn't apply.
if(!traceOut._length) return;

var globalTransforms = layout._globalTransforms || [];
var transformModules = layout._transformModules || [];

Expand Down
3 changes: 1 addition & 2 deletions src/traces/box/calc.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,11 @@ module.exports = function calc(gd, trace) {
var dPos = dv.minDiff / 2;
var posBins = makeBins(posDistinct, dPos);

var vLen = val.length;
var pLen = posDistinct.length;
var ptsPerBin = initNestedArray(pLen);

// bin pts info per position bins
for(i = 0; i < vLen; i++) {
for(i = 0; i < trace._length; i++) {
var v = val[i];
if(!isNumeric(v)) continue;

Expand Down
13 changes: 11 additions & 2 deletions src/traces/box/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,28 @@ function supplyDefaults(traceIn, traceOut, defaultColor, layout) {
function handleSampleDefaults(traceIn, traceOut, coerce, layout) {
var y = coerce('y');
var x = coerce('x');
var hasX = x && x.length;

var defaultOrientation;
var defaultOrientation, len;

if(y && y.length) {
defaultOrientation = 'v';
if(!x) coerce('x0');
if(hasX) {
len = Math.min(x.length, y.length);
}
else {
coerce('x0');
len = y.length;
}
} else if(x && x.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be hasX.

defaultOrientation = 'h';
coerce('y0');
len = x.length;
} else {
traceOut.visible = false;
return;
}
traceOut._length = len;

var handleCalendarDefaults = Registry.getComponentMethod('calendars', 'handleTraceDefaults');
handleCalendarDefaults(traceIn, traceOut, ['x', 'y'], layout);
Expand Down
9 changes: 4 additions & 5 deletions src/traces/carpet/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,12 @@ module.exports = function supplyDefaults(traceIn, traceOut, dfltColor, fullLayou
// corresponds to b and the second to a. This sounds backwards but ends up making sense
// the important part to know is that when you write y[j][i], j goes from 0 to b.length - 1
// and i goes from 0 to a.length - 1.
var len = handleXYDefaults(traceIn, traceOut, coerce);
var validData = handleXYDefaults(traceIn, traceOut, coerce);
if(!validData) {
traceOut.visible = false;
}

if(traceOut._cheater) {
coerce('cheaterslope');
}

if(!len) {
traceOut.visible = false;
}
};
16 changes: 15 additions & 1 deletion src/traces/carpet/xy_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,25 @@

'use strict';

var is1D = require('../../lib').is1D;

module.exports = function handleXYDefaults(traceIn, traceOut, coerce) {
var x = coerce('x');
var hasX = x && x.length;
var y = coerce('y');
var hasY = y && y.length;
if(!hasX && !hasY) return false;

traceOut._cheater = !x;

return !!x || !!y;
if((!hasX || is1D(x)) && (!hasY || is1D(y))) {
var len = hasX ? x.length : Infinity;
if(hasY) len = Math.min(len, y.length);
if(traceOut.a && traceOut.a.length) len = Math.min(len, traceOut.a.length);
if(traceOut.b && traceOut.b.length) len = Math.min(len, traceOut.b.length);
traceOut._length = len;
}
else traceOut._length = null;

return true;
};
2 changes: 1 addition & 1 deletion src/traces/choropleth/calc.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ var arraysToCalcdata = require('../scatter/arrays_to_calcdata');
var calcSelection = require('../scatter/calc_selection');

module.exports = function calc(gd, trace) {
var len = trace.locations.length;
var len = trace._length;
var calcTrace = new Array(len);

for(var i = 0; i < len; i++) {
Expand Down
15 changes: 3 additions & 12 deletions src/traces/choropleth/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,13 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
}

var locations = coerce('locations');

var len;
if(locations) len = locations.length;

if(!locations || !len) {
traceOut.visible = false;
return;
}

var z = coerce('z');
if(!Lib.isArrayOrTypedArray(z)) {

if(!(locations && locations.length && Lib.isArrayOrTypedArray(z) && z.length)) {
traceOut.visible = false;
return;
}

if(z.length > len) traceOut.z = z.slice(0, len);
traceOut._length = Math.min(locations.length, z.length);

coerce('locationmode');

Expand Down
1 change: 1 addition & 0 deletions src/traces/contourcarpet/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,5 +69,6 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
}
} else {
traceOut._defaultColor = defaultColor;
traceOut._length = null;
}
};
22 changes: 7 additions & 15 deletions src/traces/heatmap/convert_column_xyz.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,24 +13,16 @@ var Lib = require('../../lib');
var BADNUM = require('../../constants/numerical').BADNUM;

module.exports = function convertColumnData(trace, ax1, ax2, var1Name, var2Name, arrayVarNames) {
var col1 = trace[var1Name].slice(),
col2 = trace[var2Name].slice(),
textCol = trace.text,
colLen = Math.min(col1.length, col2.length),
hasColumnText = (textCol !== undefined && !Array.isArray(textCol[0])),
col1Calendar = trace[var1Name + 'calendar'],
col2Calendar = trace[var2Name + 'calendar'];
var colLen = trace._length;
var col1 = trace[var1Name].slice(0, colLen);
var col2 = trace[var2Name].slice(0, colLen);
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh so nice to .slice() only once!

var textCol = trace.text;
var hasColumnText = (textCol !== undefined && Lib.is1D(textCol));
var col1Calendar = trace[var1Name + 'calendar'];
var col2Calendar = trace[var2Name + 'calendar'];

var i, j, arrayVar, newArray, arrayVarName;

for(i = 0; i < arrayVarNames.length; i++) {
arrayVar = trace[arrayVarNames[i]];
if(arrayVar) colLen = Math.min(colLen, arrayVar.length);
}

if(colLen < col1.length) col1 = col1.slice(0, colLen);
if(colLen < col2.length) col2 = col2.slice(0, colLen);

for(i = 0; i < colLen; i++) {
col1[i] = ax1.d2c(col1[i], 0, col1Calendar);
col2[i] = ax2.d2c(col2[i], 0, col2Calendar);
Expand Down
8 changes: 6 additions & 2 deletions src/traces/heatmap/xyz_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ module.exports = function handleXYZDefaults(traceIn, traceOut, coerce, layout, x
y = coerce(yName);

// column z must be accompanied by xName and yName arrays
if(!x || !y) return 0;
if(!(x && x.length && y && y.length)) return 0;

traceOut._length = Math.min(x.length, y.length, z.length);
}
else {
x = coordDefaults(xName, coerce);
Expand All @@ -37,12 +39,14 @@ module.exports = function handleXYZDefaults(traceIn, traceOut, coerce, layout, x
if(!isValidZ(z)) return 0;

coerce('transpose');

traceOut._length = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if instead of null, 2D arrays would link a length-2 array to _length of row and column lengths? Of course, this won't solve the transform-and-2d-array-traces problems by itself, but it might help us clean up some indexing downstream logic like extracting values for hover labels and event data (e.g. here and here)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What if instead of null, 2D arrays would link a length-2 array to _length of row and column lengths?

I could imagine that being helpful but it sounds like a bit of a project in itself - not quite sure how it would mesh with the logic you linked for example, which may be at least partially after reshaping from columns to a grid. Anyway we can add that later, so I'd like to defer it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

so I'd like to defer it for now.

Sounds good.

}

var handleCalendarDefaults = Registry.getComponentMethod('calendars', 'handleTraceDefaults');
handleCalendarDefaults(traceIn, traceOut, [xName, yName], layout);

return traceOut.z.length;
return true;
};

function coordDefaults(coordStr, coerce) {
Expand Down
23 changes: 15 additions & 8 deletions src/traces/histogram/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
return Lib.coerce(traceIn, traceOut, attributes, attr, dflt);
}

var x = coerce('x'),
y = coerce('y');
var x = coerce('x');
var y = coerce('y');
var hasX = x && x.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect x && x.length is faster than Array.isArray(x) && x.length. Might be worth standardizing lib/is_array.js (as e.g. Lib.isNonEmptyArray) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

e.g. Lib.isNonEmptyArray

Ah hmm, before I do that, I want to check this against our previous behavior for trace types that don't necessarily need both coordinates specified - like scatter, which can use x0/dx along with a y array. For example if x=[], y=[1, 2, 3] we should treat that as _length=0, not 3, right? I'll need to look over our test coverage for those cases...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I looked at empty-array behavior in more detail 6fae229 some of these went away. So I've not created Lib.isNonEmptyArray for the time being.

var hasY = y && y.length;

var cumulative = coerce('cumulative.enabled');
if(cumulative) {
Expand All @@ -33,22 +35,27 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout

coerce('text');

var orientation = coerce('orientation', (y && !x) ? 'h' : 'v'),
sample = traceOut[orientation === 'v' ? 'x' : 'y'];
var orientation = coerce('orientation', (hasY && !hasX) ? 'h' : 'v');
var sampleLetter = orientation === 'v' ? 'x' : 'y';
var aggLetter = orientation === 'v' ? 'y' : 'x';
var sample = traceOut[sampleLetter];

if(!(sample && sample.length)) {
var len = (hasX && hasY) ? Math.min(x.length && y.length) : (sample || []).length;

if(!len) {
traceOut.visible = false;
return;
}

traceOut._length = len;

var handleCalendarDefaults = Registry.getComponentMethod('calendars', 'handleTraceDefaults');
handleCalendarDefaults(traceIn, traceOut, ['x', 'y'], layout);

var hasAggregationData = traceOut[orientation === 'h' ? 'x' : 'y'];
var hasAggregationData = traceOut[aggLetter];
if(hasAggregationData) coerce('histfunc');

var binDirections = (orientation === 'h') ? ['y'] : ['x'];
handleBinDefaults(traceIn, traceOut, coerce, binDirections);
handleBinDefaults(traceIn, traceOut, coerce, [sampleLetter]);

handleStyleDefaults(traceIn, traceOut, coerce, defaultColor, layout);

Expand Down
2 changes: 1 addition & 1 deletion src/traces/histogram2d/calc.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ module.exports = function calc(gd, trace) {

var i, j, n, m;

var serieslen = Math.min(x.length, y.length);
var serieslen = trace._length;
if(x.length > serieslen) x.splice(serieslen, x.length - serieslen);
if(y.length > serieslen) y.splice(serieslen, y.length - serieslen);

Expand Down
6 changes: 4 additions & 2 deletions src/traces/histogram2d/sample_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ var handleBinDefaults = require('../histogram/bin_defaults');


module.exports = function handleSampleDefaults(traceIn, traceOut, coerce, layout) {
var x = coerce('x'),
y = coerce('y');
var x = coerce('x');
var y = coerce('y');

// we could try to accept x0 and dx, etc...
// but that's a pretty weird use case.
Expand All @@ -25,6 +25,8 @@ module.exports = function handleSampleDefaults(traceIn, traceOut, coerce, layout
return;
}

traceOut._length = Math.min(x.length, y.length);

var handleCalendarDefaults = Registry.getComponentMethod('calendars', 'handleTraceDefaults');
handleCalendarDefaults(traceIn, traceOut, ['x', 'y'], layout);

Expand Down
3 changes: 3 additions & 0 deletions src/traces/mesh3d/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
}

coerce('text');

// disable 1D transforms
traceOut._length = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, mesh3d only expects 1D data, so _length could have a meaning and transforms could work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah nevermind, i/j/k and x/y/z lengths don't match for mesh3d traces. Can you add a comment about this above this line like you did for sankey traces?

};
11 changes: 10 additions & 1 deletion src/traces/pie/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,28 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
}

var coerceFont = Lib.coerceFont;
var len;

var vals = coerce('values');
var hasVals = Lib.isArrayOrTypedArray(vals) && vals.length;
var labels = coerce('labels');
if(Array.isArray(labels) && labels.length) {
len = labels.length;
if(hasVals) len = Math.min(len, vals.length);
}
if(!Array.isArray(labels)) {
if(!Lib.isArrayOrTypedArray(vals) || !vals.length) {
if(!hasVals) {
// must have at least one of vals or labels
traceOut.visible = false;
return;
}

len = vals.length;

coerce('label0');
coerce('dlabel');
}
traceOut._length = len;

var lineWidth = coerce('marker.line.width');
if(lineWidth) coerce('marker.line.color');
Expand Down
3 changes: 3 additions & 0 deletions src/traces/pointcloud/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor) {
coerce('marker.sizemax');
coerce('marker.border.color', defaultColor);
coerce('marker.border.arearatio');

// disable 1D transforms - that would defeat the purpose of this trace type, performance!
traceOut._length = null;
};
4 changes: 4 additions & 0 deletions src/traces/sankey/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,8 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
if(traceOut.node.label.some(missing)) {
Lib.warn('Some of the nodes are neither sources nor targets, they will not be displayed.');
}

// disable 1D transforms - arrays here are 1D but their lengths/meanings
// don't match, between nodes and links
traceOut._length = null;
};
2 changes: 1 addition & 1 deletion src/traces/scatter3d/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ function handleXYZDefaults(traceIn, traceOut, coerce, layout) {
if(x && y && z) {
// TODO: what happens if one is missing?
len = Math.min(x.length, y.length, z.length);
traceOut._xlength = traceOut._ylength = traceOut._zlength = len;
traceOut._length = traceOut._xlength = traceOut._ylength = traceOut._zlength = len;
}

return len;
Expand Down
4 changes: 4 additions & 0 deletions src/traces/surface/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
colorscaleDefaults(
traceIn, traceOut, layout, coerce, {prefix: '', cLetter: 'c'}
);

// disable 1D transforms - currently surface does NOT support column data like heatmap does
// you can use mesh3d for this use case, but not surface
traceOut._length = null;
};

function mapLegacy(traceIn, oldAttr, newAttr) {
Expand Down
3 changes: 3 additions & 0 deletions src/traces/table/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
coerce('cells.line.color');
coerce('cells.fill.color');
Lib.coerceFont(coerce, 'cells.font', Lib.extendFlat({}, layout.font));

// disable 1D transforms
traceOut._length = null;
};
15 changes: 14 additions & 1 deletion test/jasmine/tests/choropleth_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,26 @@ describe('Test choropleth', function() {
traceOut = {};
});

it('should slice z if it is longer than locations', function() {
it('should set _length based on locations and z but not slice', function() {
traceIn = {
locations: ['CAN', 'USA'],
z: [1, 2, 3]
};

Choropleth.supplyDefaults(traceIn, traceOut, defaultColor, layout);
expect(traceOut.z).toEqual([1, 2, 3]);
expect(traceOut.locations).toEqual(['CAN', 'USA']);
expect(traceOut._length).toBe(2);

traceIn = {
locations: ['CAN', 'USA', 'ALB'],
z: [1, 2]
};

Choropleth.supplyDefaults(traceIn, traceOut, defaultColor, layout);
expect(traceOut.z).toEqual([1, 2]);
expect(traceOut.locations).toEqual(['CAN', 'USA', 'ALB']);
expect(traceOut._length).toBe(2);
});

it('should make trace invisible if locations is not defined', function() {
Expand All @@ -51,6 +63,7 @@ describe('Test choropleth', function() {

it('should make trace invisible if z is not an array', function() {
traceIn = {
locations: ['CAN', 'USA'],
z: 'no gonna work'
};

Expand Down
Loading