Pie aggregation and event cleanup #2117

alexcjohnson · 2017-10-25T12:46:00Z

Implements #2073 and fixes #1456

Previously if we encountered duplicate labels we would just drop all but the first one. This PR just does the natural thing and sums all the values we find for each label. Additionally, you can now make a pie chart with no values array at all, in which case we just count occurrences of each label.

To support this, event data has been augmented with a pointNumbers field, which is an array of indices of points in the input data represented by the slice. The pointNumber field remains, but only if the slice came from a single input point. Other fields have been renamed to be more consistent with events on other trace types, but for backward compatibility I kept i (replaced by pointNumber and like it, only present for non-aggregated slices) and v (replaced by value) but they are destined for removal at some point.

Since the trace field to link to the trace for this pie was both nonstandard and mostly broken (#1456) I replaced it with data and fullData as in other event types, while ensuring it's always available.

apparently I got a lot farther than just defining attributes (deleted in #1152)

alexcjohnson · 2017-10-25T12:52:41Z

src/traces/pie/helpers.js

@@ -25,3 +25,16 @@ exports.formatPieValue = function formatPieValue(v, separators) {
    }
    return Lib.numSeparate(vRounded, separators);
 };
+
+exports.getFirstFilled = function getFirstFilled(array, indices) {


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?

Picking the first non-empty item is fine 👍

alexcjohnson · 2017-10-25T12:55:51Z

src/traces/pie/plot.js

+                        hoverinfo = Fx.castHoverinfo({
+                            hoverinfo: [helpers.castOption(hoverinfo, pt.pts)],
+                            _module: trace._module
+                        }, fullLayout2, 0);


@etpinard thoughts? I didn't see a reasonable way to build this into Fx.castHoverInfo since an array index already has a meaning there...

Maybe something like:

Fx.castHoverinfo = function(trace, fullLayout, pt) { if(pt.pointNumber) { // as currently } else if(pt.pointNumbers) { // new pie logic } } // instead of Fx.castHoverinfo = function(trace, fullLayout, ptNumber) { // ... }

but your hack (with that comment) is fine 👍

I think I should leave it as is for now, but if we unify this behavior with histograms (see #1984) then it may be worth making this change.

alexcjohnson · 2017-10-25T13:00:56Z

src/traces/pie/plot.js

-            return d + ' part';
-        });
-        pieParts.exit().remove();
-        pieParts.order();


Man, I totally forgot that I had written all this crap for 3D pies, I thought #1152 was all of it - but now between this commit and 679ed9c I think it's finally all gone.

In almost 2 years of open-source, no one has ever requested 3D pies. Good riddance! 🎉

alexcjohnson · 2017-10-25T13:05:20Z

src/components/fx/hover.js

@@ -155,7 +155,7 @@ exports.loneHover = function loneHover(hoverItem, opts) {

 // The actual implementation is here:
 function _hover(gd, evt, subplot, noHoverEvent) {
-    if((subplot === 'pie' || subplot === 'sankey') && !noHoverEvent) {
+    if(subplot === 'sankey' && !noHoverEvent) {


I couldn't see any purpose to having pie call Fx.hover other than throttling - which really shouldn't be an issue for pie, so I removed it from this special handling. Am I missing anything? Should we do the same for sankey and 🔪 this block entirely?

Looks like sankey does Fx.hover here and here, but if you feel adventurous, sure, go ahead and patch sankey/plot.js to 🔪 this block.

simplified sankey -> e08e37a
Also: I notice that the way we had it was only throttling event emission, not the actual item selection which is normally the slow part. Again, throttling shouldn't be an issue for either of these trace types, this is just more confirmation of that.

e08e37a is very nice. 👌

alexcjohnson · 2017-10-25T13:17:55Z

test/jasmine/tests/pie_test.js

+        });
+
+        it('should not emit a hover if you\'re dragging', function() {
+            gd._dragging = true;


It occurred to me when fixing this bit of logic, it would perhaps be better to check event.buttons instead? Seems both cleaner and more powerful, for example it would prevent hover if you drag over a plot from somewhere else on the page. So in principle perhaps all our hover handlers should contain this check?

that will need a bunch more testing and ideally rolling out to all subplots at once, so I'll defer it -> #2120

alexcjohnson · 2017-10-25T13:19:37Z

test/jasmine/tests/pie_test.js

+        it('should not emit a hover if hover is disabled', function() {
+            Plotly.relayout(gd, 'hovermode', false);
+            mouseEvent('mouseover', pointPos[0], pointPos[1]);
+            expect(futureData).toBeUndefined();


This is also different from before, but consistent with other subplot types, I believe.

Yes this is how it should be.

For the record, to get hover event data w/o visible hover labels, one should set hoverinfo: 'none' in all the traces of interest.

alexcjohnson · 2017-10-25T13:21:20Z

test/jasmine/tests/pie_test.js

+
+        it('should not emit an unhover if you didn\'t first hover', function() {
+            mouseEvent('mouseout', pointPos[0], pointPos[1]);
+            expect(futureData).toBeUndefined();


kind of a weird one... but lets say you dragged over the pie from another subplot, so there was no hover event - you shouldn't get an unhover event even if you mouse up and then mouse out of that slice.

alexcjohnson · 2017-10-25T13:32:52Z

src/traces/pie/plot.js

                    }
                }

                function handleClick() {
-                    gd._hoverdata = [pt];
-                    gd._hoverdata.trace = cd0.trace;


We probably didn't mean to attach trace as a property of the whole array 🙈

Wow. Brutal. Good catch ⚾

alexcjohnson · 2017-10-25T13:34:30Z

test/jasmine/tests/pie_test.js

@@ -400,43 +407,24 @@ describe('Test event property of interactions on a pie plot:', function() {
            click(pointPos[0], pointPos[1]);
            expect(futureData.points.length).toEqual(1);

-            var trace = futureData.points.trace;
-            expect(typeof trace).toEqual(typeof {}, 'points.trace');


We probably didn't mean to attach trace as a property of the whole array 🙈

And yet we had a test to verify that that's exactly what we did 🤔

etpinard

Solid work. I made two blocking comments ⛔

I noticed a few 🌴 opportunities, but since pie hover will always be a special case, I'm not sure it's worth the effort. Your call. I'm happy to see the castOption pattern getting more attention though.

etpinard · 2017-10-25T13:49:47Z

src/traces/pie/defaults.js

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


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

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

etpinard · 2017-10-25T13:56:49Z

src/traces/pie/plot.js

-            return d + ' part';
-        });
-        pieParts.exit().remove();
-        pieParts.order();


In almost 2 years of open-source, no one has ever requested 3D pies. Good riddance! 🎉

etpinard · 2017-10-25T14:00:36Z

src/components/fx/helpers.js

+    }
+};
+
+exports.appendArrayPointValues = function(pointData, trace, pointNumbers) {


Maybe appendArrayMultiPointValue or appendArrayMultiPointValues would more typo-proof.

... and please add jsDOC ⤴️

good call on appendArrayMultiPointValues -> with docs in
4ba0249

etpinard · 2017-10-25T14:02:43Z

src/components/fx/helpers.js

        }
    }
 };
+
+var pointKeyMap = {


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

etpinard · 2017-10-25T14:10:53Z

src/components/fx/helpers.js

+    return pointKeyMap[astr] || astr;
+}
+
+function getPointData(val, pointNumber) {


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.

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?

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

etpinard · 2017-10-25T14:20:19Z

test/jasmine/tests/pie_test.js

+        expect(point.id).toEqual(['maggie']);
+        expect(point.customdata).toEqual([{9: 10}]);
+
+        // for backward compat - i/v to be removed at some point?


Blocking: yeah, we should remove that in v2 - would you mind adding it to #420 ?

commented #420 (comment)

etpinard · 2017-10-25T14:25:02Z

test/jasmine/tests/pie_test.js

+        it('should not emit a hover if hover is disabled', function() {
+            Plotly.relayout(gd, 'hovermode', false);
+            mouseEvent('mouseover', pointPos[0], pointPos[1]);
+            expect(futureData).toBeUndefined();


Yes this is how it should be.

For the record, to get hover event data w/o visible hover labels, one should set hoverinfo: 'none' in all the traces of interest.

etpinard · 2017-10-25T14:25:47Z

test/jasmine/tests/pie_test.js

    });

    afterEach(destroyGraphDiv);

+    function checkEventData(data) {


A long-overdue test abstraction. Thanks!

etpinard · 2017-10-25T14:40:07Z

src/traces/pie/plot.js

+                        hoverinfo = Fx.castHoverinfo({
+                            hoverinfo: [helpers.castOption(hoverinfo, pt.pts)],
+                            _module: trace._module
+                        }, fullLayout2, 0);


Maybe something like:

Fx.castHoverinfo = function(trace, fullLayout, pt) { if(pt.pointNumber) { // as currently } else if(pt.pointNumbers) { // new pie logic } } // instead of Fx.castHoverinfo = function(trace, fullLayout, ptNumber) { // ... }

but your hack (with that comment) is fine 👍

etpinard · 2017-10-25T14:46:22Z

src/traces/pie/helpers.js

@@ -25,3 +25,16 @@ exports.formatPieValue = function formatPieValue(v, separators) {
    }
    return Lib.numSeparate(vRounded, separators);
 };
+
+exports.getFirstFilled = function getFirstFilled(array, indices) {


Picking the first non-empty item is fine 👍

etpinard · 2017-10-25T14:59:09Z

I should reference #1963 in this PR as now pie is locked into a hover pattern outside the realm of Fx.hover. Maybe a better solution to #1963 is persistent hover point list as discussed in #1848

etpinard · 2017-10-26T13:32:24Z

Lovely feature and solid clean up: looking great 💃

Make sure to close #2073, #1456 and #2048

alexcjohnson added 5 commits October 24, 2017 16:07

remove obsolete comments, esp. about 3d pies

679ed9c

first cut at aggregated pies

ce8946c

lint pie

d2756f1

🔪 my zeroed-out 3D pie code

ebc4d8b

apparently I got a lot farther than just defining attributes (deleted in #1152)

standardize pie events, and add internal aggregation info

5d0f3e2

alexcjohnson commented Oct 25, 2017

View reviewed changes

alexcjohnson added status: reviewable bug something broken feature something new labels Oct 25, 2017

alexcjohnson commented Oct 25, 2017

View reviewed changes

etpinard suggested changes Oct 25, 2017

View reviewed changes

simplify sankey hover events

e08e37a

alexcjohnson mentioned this pull request Oct 25, 2017

More robust handling of drag/hover interplay #2120

Closed

rename and document appendArrayMultiPointValues

4ba0249

alexcjohnson mentioned this pull request Oct 25, 2017

wishlist for potential breaking changes since v1 #420

Closed

15 tasks

jasmine test of pie agg with or without values

ee1f8c4

etpinard added this to the v1.32.0 milestone Oct 26, 2017

etpinard approved these changes Oct 26, 2017

View reviewed changes

alexcjohnson merged commit 77d0858 into master Oct 26, 2017

alexcjohnson deleted the pie-agg branch October 26, 2017 14:08

alexcjohnson mentioned this pull request Oct 26, 2017

Aggregated pie charts #2073

Closed

This was referenced Oct 26, 2017

onClick for pies slices doesn't return any trace info #1456

Closed

Custom data item aren't included in pie trace event data #2048

Closed

alexcjohnson mentioned this pull request Nov 3, 2017

Persistent point selection #2135

Merged

This was referenced Mar 12, 2018

Updates to pie chart data require click on legend to update properly plotly/react-plotly.js#62

Closed

transforms + Plotly.react bug #2470

Closed

etpinard mentioned this pull request Sep 10, 2018

Giving stroke / border on one specific marker in Pie Chart #2989

Closed

Pie aggregation and event cleanup #2117

Pie aggregation and event cleanup #2117

Conversation

alexcjohnson commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Oct 25, 2017

etpinard commented Oct 26, 2017

etpinard left a comment •

edited

Loading