ensure every trace defines _length in supplyDefaults, and abort transforms if it's falsy

alexcjohnson · alexcjohnson · commit b436d521ec87 · 2018-04-21T23:26:38.000-04:00
diff --git a/src/plots/plots.js b/src/plots/plots.js
@@ -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 || [];
 
diff --git a/src/traces/box/calc.js b/src/traces/box/calc.js
@@ -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;
 
diff --git a/src/traces/box/defaults.js b/src/traces/box/defaults.js
@@ -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) {
         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);
diff --git a/src/traces/carpet/defaults.js b/src/traces/carpet/defaults.js
@@ -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;
-    }
 };
diff --git a/src/traces/carpet/xy_defaults.js b/src/traces/carpet/xy_defaults.js
@@ -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;
 };
diff --git a/src/traces/choropleth/calc.js b/src/traces/choropleth/calc.js
@@ -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++) {
diff --git a/src/traces/choropleth/defaults.js b/src/traces/choropleth/defaults.js
@@ -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');
 
diff --git a/src/traces/contourcarpet/defaults.js b/src/traces/contourcarpet/defaults.js
@@ -69,5 +69,6 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
         }
     } else {
         traceOut._defaultColor = defaultColor;
+        traceOut._length = null;
     }
 };
diff --git a/src/traces/heatmap/convert_column_xyz.js b/src/traces/heatmap/convert_column_xyz.js
@@ -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);
+    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);
diff --git a/src/traces/heatmap/xyz_defaults.js b/src/traces/heatmap/xyz_defaults.js
@@ -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);
@@ -37,12 +39,14 @@ module.exports = function handleXYZDefaults(traceIn, traceOut, coerce, layout, x
         if(!isValidZ(z)) return 0;
 
         coerce('transpose');
+
+        traceOut._length = null;
     }
 
     var handleCalendarDefaults = Registry.getComponentMethod('calendars', 'handleTraceDefaults');
     handleCalendarDefaults(traceIn, traceOut, [xName, yName], layout);
 
-    return traceOut.z.length;
+    return true;
 };
 
 function coordDefaults(coordStr, coerce) {
diff --git a/src/traces/histogram/defaults.js b/src/traces/histogram/defaults.js
@@ -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;
+    var hasY = y && y.length;
 
     var cumulative = coerce('cumulative.enabled');
     if(cumulative) {
@@ -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);
 
diff --git a/src/traces/histogram2d/calc.js b/src/traces/histogram2d/calc.js
@@ -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);
 
diff --git a/src/traces/histogram2d/sample_defaults.js b/src/traces/histogram2d/sample_defaults.js
@@ -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.
@@ -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);
 
diff --git a/src/traces/mesh3d/defaults.js b/src/traces/mesh3d/defaults.js
@@ -86,4 +86,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
     }
 
     coerce('text');
+
+    // disable 1D transforms
+    traceOut._length = null;
 };
diff --git a/src/traces/pie/defaults.js b/src/traces/pie/defaults.js
@@ -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');
diff --git a/src/traces/pointcloud/defaults.js b/src/traces/pointcloud/defaults.js
@@ -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;
 };
diff --git a/src/traces/sankey/defaults.js b/src/traces/sankey/defaults.js
@@ -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;
 };
diff --git a/src/traces/scatter3d/defaults.js b/src/traces/scatter3d/defaults.js
@@ -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;
diff --git a/src/traces/surface/defaults.js b/src/traces/surface/defaults.js
@@ -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) {
diff --git a/src/traces/table/defaults.js b/src/traces/table/defaults.js
@@ -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;
 };
diff --git a/test/jasmine/tests/choropleth_test.js b/test/jasmine/tests/choropleth_test.js
@@ -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() {
@@ -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'
             };
 
diff --git a/test/jasmine/tests/heatmap_test.js b/test/jasmine/tests/heatmap_test.js
diff --git a/test/jasmine/tests/histogram_test.js b/test/jasmine/tests/histogram_test.js
diff --git a/test/jasmine/tests/plot_api_test.js b/test/jasmine/tests/plot_api_test.js
diff --git a/test/jasmine/tests/plots_test.js b/test/jasmine/tests/plots_test.js
diff --git a/test/jasmine/tests/transform_multi_test.js b/test/jasmine/tests/transform_multi_test.js

Original file line number	Diff line number	Diff line change
`@@ -69,5 +69,6 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout`
`69`	`69`	`}`
`70`	`70`	`} else {`
`71`	`71`	`traceOut._defaultColor = defaultColor;`
	`72`	`+ traceOut._length = null;`
`72`	`73`	`}`
`73`	`74`	`};`