From dfc14392826df296d3f74120ed408ef88b410f83 Mon Sep 17 00:00:00 2001 From: archmoj Date: Fri, 6 Dec 2019 18:34:53 -0500 Subject: [PATCH 1/2] handle invalid values and zero totals for pie and funnelarea --- src/traces/funnelarea/defaults.js | 18 +++++---- src/traces/pie/calc.js | 13 +++---- src/traces/pie/defaults.js | 54 ++++++++++++++++++++++----- src/traces/pie/index.js | 2 +- test/jasmine/tests/funnelarea_test.js | 38 +++++++++++++++++++ test/jasmine/tests/pie_test.js | 38 +++++++++++++++++++ 6 files changed, 138 insertions(+), 25 deletions(-) diff --git a/src/traces/funnelarea/defaults.js b/src/traces/funnelarea/defaults.js index a138b8d51ea..e1a37f99303 100644 --- a/src/traces/funnelarea/defaults.js +++ b/src/traces/funnelarea/defaults.js @@ -12,22 +12,24 @@ var Lib = require('../../lib'); var attributes = require('./attributes'); var handleDomainDefaults = require('../../plots/domain').defaults; var handleText = require('../bar/defaults').handleText; +var handleLabelsAndValues = require('../pie/defaults').handleLabelsAndValues; module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout) { function coerce(attr, dflt) { return Lib.coerce(traceIn, traceOut, attributes, attr, dflt); } - var len; - var vals = coerce('values'); - var hasVals = Lib.isArrayOrTypedArray(vals); var labels = coerce('labels'); - if(Array.isArray(labels)) { - len = labels.length; - if(hasVals) len = Math.min(len, vals.length); - } else if(hasVals) { - len = vals.length; + var values = coerce('values'); + var res = handleLabelsAndValues(labels, values); + var len = res.len; + traceOut._hasLabels = res.hasLabels; + traceOut._hasValues = res.hasValues; + + if(!traceOut._hasLabels && + traceOut._hasValues + ) { coerce('label0'); coerce('dlabel'); } diff --git a/src/traces/pie/calc.js b/src/traces/pie/calc.js index 89b361d29fc..b280566bad0 100644 --- a/src/traces/pie/calc.js +++ b/src/traces/pie/calc.js @@ -9,7 +9,6 @@ 'use strict'; var isNumeric = require('fast-isnumeric'); -var isArrayOrTypedArray = require('../../lib').isArrayOrTypedArray; var tinycolor = require('tinycolor2'); var Color = require('../../components/color'); @@ -25,26 +24,26 @@ function calc(gd, trace) { var labels = trace.labels; var colors = trace.marker.colors || []; var vals = trace.values; - var hasVals = isArrayOrTypedArray(vals) && vals.length; + var len = trace._length; + var hasValues = trace._hasValues && len; var i, pt; if(trace.dlabel) { - labels = new Array(vals.length); - for(i = 0; i < vals.length; i++) { + labels = []; + for(i = 0; i < len; i++) { labels[i] = String(trace.label0 + i * trace.dlabel); } } var allThisTraceLabels = {}; var pullColor = makePullColorFn(fullLayout['_' + trace.type + 'colormap']); - var seriesLen = (hasVals ? vals : labels).length; var vTotal = 0; var isAggregated = false; - for(i = 0; i < seriesLen; i++) { + for(i = 0; i < len; i++) { var v, label, hidden; - if(hasVals) { + if(hasValues) { v = vals[i]; if(!isNumeric(v)) continue; v = +v; diff --git a/src/traces/pie/defaults.js b/src/traces/pie/defaults.js index 0d7cfc47343..e67da0f7a51 100644 --- a/src/traces/pie/defaults.js +++ b/src/traces/pie/defaults.js @@ -13,21 +13,52 @@ var attributes = require('./attributes'); var handleDomainDefaults = require('../../plots/domain').defaults; var handleText = require('../bar/defaults').handleText; -module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout) { +function handleLabelsAndValues(labels, values) { + var hasLabels = Array.isArray(labels); + var hasValues = Lib.isArrayOrTypedArray(values); + var len = Math.min( + hasLabels ? labels.length : Infinity, + hasValues ? values.length : Infinity + ); + + if(!isFinite(len)) len = 0; + + if(len && hasValues) { + var sum = 0; + for(var i = 0; i < len; i++) { + var v = +values[i]; + if(v < 0) { + sum = 0; + break; + } + sum += v; + } + if(!sum) len = 0; + } + + return { + hasLabels: hasLabels, + hasValues: hasValues, + len: len + }; +} + +function supplyDefaults(traceIn, traceOut, defaultColor, layout) { function coerce(attr, dflt) { return Lib.coerce(traceIn, traceOut, attributes, attr, dflt); } - var len; - var vals = coerce('values'); - var hasVals = Lib.isArrayOrTypedArray(vals); var labels = coerce('labels'); - if(Array.isArray(labels)) { - len = labels.length; - if(hasVals) len = Math.min(len, vals.length); - } else if(hasVals) { - len = vals.length; + var values = coerce('values'); + + var res = handleLabelsAndValues(labels, values); + var len = res.len; + traceOut._hasLabels = res.hasLabels; + traceOut._hasValues = res.hasValues; + if(!traceOut._hasLabels && + traceOut._hasValues + ) { coerce('label0'); coerce('dlabel'); } @@ -86,4 +117,9 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout coerce('direction'); coerce('rotation'); coerce('pull'); +} + +module.exports = { + handleLabelsAndValues: handleLabelsAndValues, + supplyDefaults: supplyDefaults }; diff --git a/src/traces/pie/index.js b/src/traces/pie/index.js index 0597ab64f2e..4265a55cca4 100644 --- a/src/traces/pie/index.js +++ b/src/traces/pie/index.js @@ -10,7 +10,7 @@ module.exports = { attributes: require('./attributes'), - supplyDefaults: require('./defaults'), + supplyDefaults: require('./defaults').supplyDefaults, supplyLayoutDefaults: require('./layout_defaults'), layoutAttributes: require('./layout_attributes'), diff --git a/test/jasmine/tests/funnelarea_test.js b/test/jasmine/tests/funnelarea_test.js index 67e2fa6a015..1ed6a127db1 100644 --- a/test/jasmine/tests/funnelarea_test.js +++ b/test/jasmine/tests/funnelarea_test.js @@ -71,6 +71,44 @@ describe('Funnelarea defaults', function() { expect(out.visible).toBe(false); }); + it('allows falsy json values to skip but does not allow bad values and zero sum', function() { + var out; + + [null, 0, '', false].forEach(function(e) { + out = _supply({type: 'pie', values: [1, e, 3]}); + expect(out.visible).toBe(true, e); + expect(out._length).toBe(3, e); + + out = _supply({type: 'pie', values: [1, e]}); + expect(out.visible).toBe(true, e); + expect(out._length).toBe(2, e); + + out = _supply({type: 'pie', values: [0, e]}); + expect(out.visible).toBe(false, e); + expect(out._length).toBe(undefined, e); + + out = _supply({type: 'pie', values: [e]}); + expect(out.visible).toBe(false, e); + expect(out._length).toBe(undefined, e); + }); + + ['not a positive value', '-1', -1, NaN, undefined].forEach(function(e) { + out = _supply({type: 'pie', values: [0, e]}); + expect(out.visible).toBe(false, e); + expect(out._length).toBe(undefined, e); + + out = _supply({type: 'pie', values: [1, e]}); + expect(out.visible).toBe(false, e); + expect(out._length).toBe(undefined, e); + }); + + ['1', '+1', '1e1'].forEach(function(e) { + out = _supply({type: 'pie', values: [0, e]}); + expect(out.visible).toBe(true, e); + expect(out._length).toBe(2, e); + }); + }); + it('is marked invisible if either labels or values is empty', function() { var out = _supply({type: 'funnelarea', labels: [], values: [1, 2]}); expect(out.visible).toBe(false); diff --git a/test/jasmine/tests/pie_test.js b/test/jasmine/tests/pie_test.js index b8303f19897..7661107b114 100644 --- a/test/jasmine/tests/pie_test.js +++ b/test/jasmine/tests/pie_test.js @@ -55,6 +55,44 @@ describe('Pie defaults', function() { expect(out.visible).toBe(false); }); + it('allows falsy json values to skip but does not allow bad values and zero sum', function() { + var out; + + [null, 0, '', false].forEach(function(e) { + out = _supply({type: 'pie', values: [1, e, 3]}); + expect(out.visible).toBe(true, e); + expect(out._length).toBe(3, e); + + out = _supply({type: 'pie', values: [1, e]}); + expect(out.visible).toBe(true, e); + expect(out._length).toBe(2, e); + + out = _supply({type: 'pie', values: [0, e]}); + expect(out.visible).toBe(false, e); + expect(out._length).toBe(undefined, e); + + out = _supply({type: 'pie', values: [e]}); + expect(out.visible).toBe(false, e); + expect(out._length).toBe(undefined, e); + }); + + ['not a positive value', '-1', -1, NaN, -Infinity, undefined].forEach(function(e) { + out = _supply({type: 'pie', values: [0, e]}); + expect(out.visible).toBe(false, e); + expect(out._length).toBe(undefined, e); + + out = _supply({type: 'pie', values: [1, e]}); + expect(out.visible).toBe(false, e); + expect(out._length).toBe(undefined, e); + }); + + ['1', '+1', '1e1'].forEach(function(e) { + out = _supply({type: 'pie', values: [0, e]}); + expect(out.visible).toBe(true, e); + expect(out._length).toBe(2, e); + }); + }); + it('is marked invisible if either labels or values is empty', function() { var out = _supply({type: 'pie', labels: [], values: [1, 2]}); expect(out.visible).toBe(false); From 1967fd459ef07a8e09edc52a997d74539bd9914a Mon Sep 17 00:00:00 2001 From: archmoj Date: Tue, 7 Jan 2020 12:58:38 -0500 Subject: [PATCH 2/2] return as soon as finding a valid positive number - apply isNumeric test - should skip Infinity - no need to compuet sum - declare known size of Array --- src/traces/pie/calc.js | 2 +- src/traces/pie/defaults.js | 12 +++++------ test/jasmine/tests/funnelarea_test.js | 31 ++++++++++++++++----------- test/jasmine/tests/pie_test.js | 31 ++++++++++++++++----------- 4 files changed, 43 insertions(+), 33 deletions(-) diff --git a/src/traces/pie/calc.js b/src/traces/pie/calc.js index bfe8eb9540c..c16a748008e 100644 --- a/src/traces/pie/calc.js +++ b/src/traces/pie/calc.js @@ -30,7 +30,7 @@ function calc(gd, trace) { var i, pt; if(trace.dlabel) { - labels = []; + labels = new Array(len); for(i = 0; i < len; i++) { labels[i] = String(trace.label0 + i * trace.dlabel); } diff --git a/src/traces/pie/defaults.js b/src/traces/pie/defaults.js index 38ce153cc82..174b9964ec3 100644 --- a/src/traces/pie/defaults.js +++ b/src/traces/pie/defaults.js @@ -8,6 +8,7 @@ 'use strict'; +var isNumeric = require('fast-isnumeric'); var Lib = require('../../lib'); var attributes = require('./attributes'); var handleDomainDefaults = require('../../plots/domain').defaults; @@ -24,16 +25,15 @@ function handleLabelsAndValues(labels, values) { if(!isFinite(len)) len = 0; if(len && hasValues) { - var sum = 0; + var hasPositive; for(var i = 0; i < len; i++) { - var v = +values[i]; - if(v < 0) { - sum = 0; + var v = values[i]; + if(isNumeric(v) && v > 0) { + hasPositive = true; break; } - sum += v; } - if(!sum) len = 0; + if(!hasPositive) len = 0; } return { diff --git a/test/jasmine/tests/funnelarea_test.js b/test/jasmine/tests/funnelarea_test.js index b4ab55182b9..e0c0b7e0288 100644 --- a/test/jasmine/tests/funnelarea_test.js +++ b/test/jasmine/tests/funnelarea_test.js @@ -71,10 +71,21 @@ describe('Funnelarea defaults', function() { expect(out.visible).toBe(false); }); - it('allows falsy json values to skip but does not allow bad values and zero sum', function() { - var out; + it('skip negatives and non-JSON values and avoid zero total', function() { + [ + -1, '-1', + 0, '0', + false, 'false', + true, 'true', + null, 'null', + NaN, 'NaN', + -Infinity, '-Infinity', + Infinity, 'Infinity', + undefined, 'undefined', + '', [], {} + ].forEach(function(e) { + var out; - [null, 0, '', false].forEach(function(e) { out = _supply({type: 'pie', values: [1, e, 3]}); expect(out.visible).toBe(true, e); expect(out._length).toBe(3, e); @@ -91,18 +102,12 @@ describe('Funnelarea defaults', function() { expect(out.visible).toBe(false, e); expect(out._length).toBe(undefined, e); }); + }); - ['not a positive value', '-1', -1, NaN, undefined].forEach(function(e) { - out = _supply({type: 'pie', values: [0, e]}); - expect(out.visible).toBe(false, e); - expect(out._length).toBe(undefined, e); - - out = _supply({type: 'pie', values: [1, e]}); - expect(out.visible).toBe(false, e); - expect(out._length).toBe(undefined, e); - }); - + it('convert positive numbers in string format', function() { ['1', '+1', '1e1'].forEach(function(e) { + var out; + out = _supply({type: 'pie', values: [0, e]}); expect(out.visible).toBe(true, e); expect(out._length).toBe(2, e); diff --git a/test/jasmine/tests/pie_test.js b/test/jasmine/tests/pie_test.js index 7073cdec466..61ea09fde3c 100644 --- a/test/jasmine/tests/pie_test.js +++ b/test/jasmine/tests/pie_test.js @@ -55,10 +55,21 @@ describe('Pie defaults', function() { expect(out.visible).toBe(false); }); - it('allows falsy json values to skip but does not allow bad values and zero sum', function() { - var out; + it('skip negatives and non-JSON values and avoid zero total', function() { + [ + -1, '-1', + 0, '0', + false, 'false', + true, 'true', + null, 'null', + NaN, 'NaN', + -Infinity, '-Infinity', + Infinity, 'Infinity', + undefined, 'undefined', + '', [], {} + ].forEach(function(e) { + var out; - [null, 0, '', false].forEach(function(e) { out = _supply({type: 'pie', values: [1, e, 3]}); expect(out.visible).toBe(true, e); expect(out._length).toBe(3, e); @@ -75,18 +86,12 @@ describe('Pie defaults', function() { expect(out.visible).toBe(false, e); expect(out._length).toBe(undefined, e); }); + }); - ['not a positive value', '-1', -1, NaN, -Infinity, undefined].forEach(function(e) { - out = _supply({type: 'pie', values: [0, e]}); - expect(out.visible).toBe(false, e); - expect(out._length).toBe(undefined, e); - - out = _supply({type: 'pie', values: [1, e]}); - expect(out.visible).toBe(false, e); - expect(out._length).toBe(undefined, e); - }); - + it('convert positive numbers in string format', function() { ['1', '+1', '1e1'].forEach(function(e) { + var out; + out = _supply({type: 'pie', values: [0, e]}); expect(out.visible).toBe(true, e); expect(out._length).toBe(2, e);