Skip to content

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

Merged
archmoj merged 3 commits into
masterfrom
fix4414-pie-handle-invalid-values-and-zero-total
Jan 7, 2020
Merged

Handle invalid values and zero totals for pie and funnelarea#4416
archmoj merged 3 commits into
masterfrom
fix4414-pie-handle-invalid-values-and-zero-total

Conversation

@archmoj

Copy link
Copy Markdown
Contributor

Fixes#4414

Before
After

@plotly/plotly_js

@archmojarchmoj added bug something broken status: reviewable labels Dec 7, 2019
Comment threadsrc/traces/pie/calc.js Outdated
if(trace.dlabel) {
labels = new Array(vals.length);
for(i = 0; i < vals.length; i++) {
labels = [];

@etpinardetpinardDec 9, 2019

Copy link
Copy Markdown
Contributor

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.

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

like you have here.

Copy link
Copy Markdown
ContributorAuthor

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.

Comment threadsrc/traces/pie/defaults.js Outdated
Comment on lines +30 to +32
if(v < 0) {
sum = 0;
break;

@etpinardetpinardDec 9, 2019

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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.

- apply isNumeric test
- should skip Infinity
- no need to compuet sum
- declare known size of Array
@etpinard

Copy link
Copy Markdown
Contributor

Nicely done - 💃 !

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugsomething broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pie should handle case of zero total values

2 participants

@archmoj@etpinard