Skip to content

Handle invalid values and zero totals for pie and funnelarea #4416

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.

Sign up for GitHub

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 3 commits into from
Jan 7, 2020
Merged

Handle invalid values and zero totals for pie and funnelarea #4416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dfc1439
handle invalid values and zero totals for pie and funnelarea
archmoj Dec 6, 2019
7d2af31
Merge branch 'master' into fix4414-pie-handle-invalid-values-and-zero…
archmoj Jan 7, 2020
1967fd4
return as soon as finding a valid positive number
archmoj Jan 7, 2020
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
18 changes: 10 additions & 8 deletions src/traces/funnelarea/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
Expand Down
13 changes: 6 additions & 7 deletions src/traces/pie/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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 = new Array(len);
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;
Expand Down
54 changes: 45 additions & 9 deletions src/traces/pie/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,57 @@

'use strict';

var isNumeric = require('fast-isnumeric');
var Lib = require('../../lib');
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 hasPositive;
for(var i = 0; i < len; i++) {
var v = values[i];
if(isNumeric(v) && v > 0) {
hasPositive = true;
break;
}
}
if(!hasPositive) len = 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Moreover, you're potentially looping here over all the values items. This is something we usually defer to the calc step to keep supplyDefaults performance scaling as the number of attributes NOT the number of data array items.

As pie-like trace usually don't have that many data array items, this is not a big deal, but still could you move some of this logic to the calc step?

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 call. Done in 1967fd4.


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');
}
Expand Down Expand Up @@ -90,4 +121,9 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
coerce('direction');
coerce('rotation');
coerce('pull');
}

module.exports = {
handleLabelsAndValues: handleLabelsAndValues,
supplyDefaults: supplyDefaults
};
2 changes: 1 addition & 1 deletion src/traces/pie/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

module.exports = {
attributes: require('./attributes'),
supplyDefaults: require('./defaults'),
supplyDefaults: require('./defaults').supplyDefaults,
supplyLayoutDefaults: require('./layout_defaults'),
layoutAttributes: require('./layout_attributes'),

Expand Down
43 changes: 43 additions & 0 deletions test/jasmine/tests/funnelarea_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,49 @@ describe('Funnelarea defaults', function() {
expect(out.visible).toBe(false);
});

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;

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);
});
});

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);
});
});

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);
Expand Down
43 changes: 43 additions & 0 deletions test/jasmine/tests/pie_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,49 @@ describe('Pie defaults', function() {
expect(out.visible).toBe(false);
});

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;

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);
});
});

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);
});
});

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);
Expand Down