Skip to content

Pie aggregation and event cleanup #2117

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 8 commits into from
Oct 26, 2017
Merged

Pie aggregation and event cleanup #2117

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
679ed9c
remove obsolete comments, esp. about 3d pies
alexcjohnson Oct 24, 2017
ce8946c
first cut at aggregated pies
alexcjohnson Oct 24, 2017
d2756f1
lint pie
alexcjohnson Oct 24, 2017
ebc4d8b
:hocho: my zeroed-out 3D pie code
alexcjohnson Oct 24, 2017
5d0f3e2
standardize pie events, and add internal aggregation info
alexcjohnson Oct 25, 2017
e08e37a
simplify sankey hover events
alexcjohnson Oct 25, 2017
4ba0249
rename and document appendArrayMultiPointValues
alexcjohnson Oct 25, 2017
ee1f8c4
jasmine test of pie agg with or without values
alexcjohnson Oct 25, 2017
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
13 changes: 11 additions & 2 deletions src/traces/pie/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ module.exports = {
labels: {
valType: 'data_array',
editType: 'calc',
description: 'Sets the sector labels.'
description: [
'Sets the sector labels.',
'If `labels` entries are duplicated, we sum associated `values`',
'or simply count occurrences if `values` is not provided.',
'For other array attributes (including color) we use the first',
'non-empty entry among all occurrences of the label.'
].join(' ')
},
// equivalent of x0 and dx, if label is missing
label0: {
Expand All @@ -50,7 +56,10 @@ module.exports = {
values: {
valType: 'data_array',
editType: 'calc',
description: 'Sets the values of the sectors of this pie chart.'
description: [
'Sets the values of the sectors of this pie chart.',
'If omitted, we count occurrences of each label.'
].join(' ')
},

marker: {
Expand Down
137 changes: 78 additions & 59 deletions src/traces/pie/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,18 @@ var Color = require('../../components/color');
var helpers = require('./helpers');

module.exports = function calc(gd, trace) {
var vals = trace.values,
labels = trace.labels,
cd = [],
fullLayout = gd._fullLayout,
colorMap = fullLayout._piecolormap,
allThisTraceLabels = {},
needDefaults = false,
vTotal = 0,
hiddenLabels = fullLayout.hiddenlabels || [],
i,
v,
label,
color,
hidden,
pt;
var vals = trace.values;
var hasVals = Array.isArray(vals) && vals.length;
var labels = trace.labels;
var colors = trace.marker.colors;
var cd = [];
var fullLayout = gd._fullLayout;
var colorMap = fullLayout._piecolormap;
var allThisTraceLabels = {};
var vTotal = 0;
var hiddenLabels = fullLayout.hiddenlabels || [];

var i, v, label, hidden, pt;

if(trace.dlabel) {
labels = new Array(vals.length);
Expand All @@ -38,46 +35,60 @@ module.exports = function calc(gd, trace) {
}
}

for(i = 0; i < vals.length; i++) {
v = vals[i];
if(!isNumeric(v)) continue;
v = +v;
if(v < 0) continue;
function pullColor(color, label) {
if(!color) return false;

color = tinycolor(color);
if(!color.isValid()) return false;

color = Color.addOpacity(color, color.getAlpha());
if(!colorMap[label]) colorMap[label] = color;

return color;
}

var seriesLen = (hasVals ? vals : labels).length;

for(i = 0; i < seriesLen; i++) {
if(hasVals) {
v = vals[i];
if(!isNumeric(v)) continue;
v = +v;
if(v < 0) continue;
}
else v = 1;

label = labels[i];
if(label === undefined || label === '') label = i;
label = String(label);
// only take the first occurrence of any given label.
// TODO: perhaps (optionally?) sum values for a repeated label?
if(allThisTraceLabels[label] === undefined) allThisTraceLabels[label] = true;
else continue;

color = tinycolor(trace.marker.colors[i]);
if(color.isValid()) {
color = Color.addOpacity(color, color.getAlpha());
if(!colorMap[label]) {
colorMap[label] = color;
}
}
// have we seen this label and assigned a color to it in a previous trace?
else if(colorMap[label]) color = colorMap[label];
// color needs a default - mark it false, come back after sorting
else {
color = false;
needDefaults = true;
}

hidden = hiddenLabels.indexOf(label) !== -1;
var thisLabelIndex = allThisTraceLabels[label];
if(thisLabelIndex === undefined) {
allThisTraceLabels[label] = cd.length;

if(!hidden) vTotal += v;
hidden = hiddenLabels.indexOf(label) !== -1;

cd.push({
v: v,
label: label,
color: color,
i: i,
hidden: hidden
});
if(!hidden) vTotal += v;

cd.push({
v: v,
label: label,
color: pullColor(colors[i]),
i: i,
pts: [i],
hidden: hidden
});
}
else {
pt = cd[thisLabelIndex];
pt.v += v;
pt.pts.push(i);
if(!pt.hidden) vTotal += v;

if(pt.color === false && colors[i]) {
pt.color = pullColor(colors[i], label);
}
}
}

if(trace.sort) cd.sort(function(a, b) { return b.v - a.v; });
Expand All @@ -88,10 +99,14 @@ module.exports = function calc(gd, trace) {
* in the order slices will be displayed
*/

if(needDefaults) {
for(i = 0; i < cd.length; i++) {
pt = cd[i];
if(pt.color === false) {
for(i = 0; i < cd.length; i++) {
pt = cd[i];
if(pt.color === false) {
// have we seen this label and assigned a color to it in a previous trace?
if(colorMap[pt.label]) {
pt.color = colorMap[pt.label];
}
else {
colorMap[pt.label] = pt.color = nextDefaultColor(fullLayout._piedefaultcolorcount);
fullLayout._piedefaultcolorcount++;
}
Expand All @@ -103,17 +118,21 @@ module.exports = function calc(gd, trace) {

// now insert text
if(trace.textinfo && trace.textinfo !== 'none') {
var hasLabel = trace.textinfo.indexOf('label') !== -1,
hasText = trace.textinfo.indexOf('text') !== -1,
hasValue = trace.textinfo.indexOf('value') !== -1,
hasPercent = trace.textinfo.indexOf('percent') !== -1,
separators = fullLayout.separators,
thisText;
var hasLabel = trace.textinfo.indexOf('label') !== -1;
var hasText = trace.textinfo.indexOf('text') !== -1;
var hasValue = trace.textinfo.indexOf('value') !== -1;
var hasPercent = trace.textinfo.indexOf('percent') !== -1;
var separators = fullLayout.separators;

var thisText;

for(i = 0; i < cd.length; i++) {
pt = cd[i];
thisText = hasLabel ? [pt.label] : [];
if(hasText && trace.text[pt.i]) thisText.push(trace.text[pt.i]);
if(hasText) {
var texti = helpers.getFirstFilled(trace.text, pt.pts);
if(texti) thisText.push(texti);
}
if(hasValue) thisText.push(helpers.formatPieValue(pt.v, separators));
if(hasPercent) thisText.push(helpers.formatPiePercent(pt.v / vTotal, separators));
pt.text = thisText.join('<br>');
Expand Down
27 changes: 8 additions & 19 deletions src/traces/pie/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,14 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
var coerceFont = Lib.coerceFont;

var vals = coerce('values');
if(!Array.isArray(vals) || !vals.length) {
traceOut.visible = false;
return;
}

var labels = coerce('labels');
if(!Array.isArray(labels)) {
if(!Array.isArray(vals) || !vals.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Blocking: Would you mind locking this down in a jasmine test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure -> ee1f8c4
It was done in the image test I added, but nice to have it in jasmine too.

// must have at least one of vals or labels
traceOut.visible = false;
return;
}

coerce('label0');
coerce('dlabel');
}
Expand All @@ -34,14 +35,10 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
if(lineWidth) coerce('marker.line.color');

var colors = coerce('marker.colors');
if(!Array.isArray(colors)) traceOut.marker.colors = []; // later this will get padded with default colors
if(!Array.isArray(colors)) traceOut.marker.colors = [];

coerce('scalegroup');
// TODO: tilt, depth, and hole all need to be coerced to the same values within a scaleegroup
// (ideally actually, depth would get set the same *after* scaling, ie the same absolute depth)
// and if colors aren't specified we should match these up - potentially even if separate pies
// are NOT in the same sharegroup

// TODO: hole needs to be coerced to the same value within a scaleegroup

var textData = coerce('text');
var textInfo = coerce('textinfo', Array.isArray(textData) ? 'text+percent' : 'percent');
Expand All @@ -63,14 +60,6 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
coerce('domain.x');
coerce('domain.y');

// 3D attributes commented out until I finish them in a later PR
// var tilt = coerce('tilt');
// if(tilt) {
// coerce('tiltaxis');
// coerce('depth');
// coerce('shading');
// }

coerce('hole');

coerce('sort');
Expand Down
13 changes: 13 additions & 0 deletions src/traces/pie/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,16 @@ exports.formatPieValue = function formatPieValue(v, separators) {
}
return Lib.numSeparate(vRounded, separators);
};

exports.getFirstFilled = function getFirstFilled(array, indices) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For arrayOk attributes, as well as slice color, I chose to take the first non-empty item corresponding to each label. I suppose in principle we could refine that further to the first valid item for each label?

Copy link
Contributor

Choose a reason for hiding this comment

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

Picking the first non-empty item is fine 👍

if(!Array.isArray(array)) return;
for(var i = 0; i < indices.length; i++) {
var v = array[indices[i]];
if(v || v === 0) return v;
}
};

exports.castOption = function castOption(item, indices) {
if(Array.isArray(item)) return exports.getFirstFilled(item, indices);
else if(item) return item;
};
Loading