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 all 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
69 changes: 58 additions & 11 deletions src/components/fx/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ exports.quadrature = function quadrature(dx, dy) {
*
* @param {object} pointData : point data object (gets mutated here)
* @param {object} trace : full trace object
* @param {number} pointNumber : point number
* @param {number|Array(number)} pointNumber : point number. May be a length-2 array
* [row, col] to dig into 2D arrays
*/
exports.appendArrayPointValue = function(pointData, trace, pointNumber) {
var arrayAttrs = trace._arrayAttrs;
Expand All @@ -100,22 +101,68 @@ exports.appendArrayPointValue = function(pointData, trace, pointNumber) {

for(var i = 0; i < arrayAttrs.length; i++) {
var astr = arrayAttrs[i];
var key;
var key = getPointKey(astr);

if(astr === 'ids') key = 'id';
else if(astr === 'locations') key = 'location';
else key = astr;
if(pointData[key] === undefined) {
var val = Lib.nestedProperty(trace, astr).get();
var pointVal = getPointData(val, pointNumber);

if(pointVal !== undefined) pointData[key] = pointVal;
}
}
};

/**
* Appends values inside array attributes corresponding to given point number array
* For use when pointData references a plot entity that arose (or potentially arose)
* from multiple points in the input data
*
* @param {object} pointData : point data object (gets mutated here)
* @param {object} trace : full trace object
* @param {Array(number)|Array(Array(number))} pointNumbers : Array of point numbers.
* Each entry in the array may itself be a length-2 array [row, col] to dig into 2D arrays
*/
exports.appendArrayMultiPointValues = function(pointData, trace, pointNumbers) {
var arrayAttrs = trace._arrayAttrs;

if(!arrayAttrs) {
return;
}

for(var i = 0; i < arrayAttrs.length; i++) {
var astr = arrayAttrs[i];
var key = getPointKey(astr);

if(pointData[key] === undefined) {
var val = Lib.nestedProperty(trace, astr).get();
var keyVal = new Array(pointNumbers.length);

if(Array.isArray(pointNumber)) {
if(Array.isArray(val) && Array.isArray(val[pointNumber[0]])) {
pointData[key] = val[pointNumber[0]][pointNumber[1]];
}
} else {
pointData[key] = val[pointNumber];
for(var j = 0; j < pointNumbers.length; j++) {
keyVal[j] = getPointData(val, pointNumbers[j]);
}
pointData[key] = keyVal;
}
}
};

var pointKeyMap = {
Copy link
Contributor

Choose a reason for hiding this comment

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

✨ at some point we might want to move this to the attribute declarations.

ids: 'id',
locations: 'location',
labels: 'label',
values: 'value',
'marker.colors': 'color'
};

function getPointKey(astr) {
return pointKeyMap[astr] || astr;
}

function getPointData(val, pointNumber) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This thing is very similar to Lib.castOption. I wonder if we could combine them? Note that getPointData should not fallback to trace-wide properties.

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 the multipoint case it's nice to keep the nestedProperty call out of this function, as that would slow it down dramatically. Between that and the fact that, as you mention, this one does not return trace-wide properties (it would return undefined but in fact it never gets there as it's iterating over arrayAttrs) these functions seem fairly distinct - though perhaps Lib.castOption could call this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, it might be nice to move getPointData to Lib at some point, but let's keep this as is for now.

if(Array.isArray(pointNumber)) {
if(Array.isArray(val) && Array.isArray(val[pointNumber[0]])) {
return val[pointNumber[0]][pointNumber[1]];
}
} else {
return val[pointNumber];
}
}
8 changes: 0 additions & 8 deletions src/components/fx/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,6 @@ exports.loneHover = function loneHover(hoverItem, opts) {

// The actual implementation is here:
function _hover(gd, evt, subplot, noHoverEvent) {
if((subplot === 'pie' || subplot === 'sankey') && !noHoverEvent) {
gd.emit('plotly_hover', {
event: evt.originalEvent,
points: [evt]
});
return;
}

if(!subplot) subplot = 'xy';

// if the user passed in an array of subplots,
Expand Down
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
Loading