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 1 commit
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 = [];
Copy link
Contributor

@etpinard etpinard Dec 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What's wrong with labels = new Array(len);? I'm not a fan an implicit array-pushing e.g.

var arr = [];
for(var i = 0; i < N; i++) {
  arr[i] = i;
}

like you have here.

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.

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 @@ -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;
Copy link
Contributor

@etpinard etpinard Dec 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm. Don't you mean continue; here instead of break; ?

If I understand correctly, this makes any trace with one (or more) negative values have _length=0 ?

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.

}
sum += v;
}
if(!sum) 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 @@ -86,4 +117,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
38 changes: 38 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,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);
Expand Down
38 changes: 38 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,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);
Expand Down