-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Stacked Area Charts #2960
Stacked Area Charts #2960
Conversation
@@ -75,7 +75,6 @@ | |||
{ | |||
"x": [1.5], | |||
"y": [1.25], | |||
"fill": "tonexty", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this change I think is actually required due to a change in the stacked area commit be38e93#diff-33c02cd37e7a4c951059a3c93221ac4eR175 - we were accidentally treating a length-1 trace as filling to itself (since its start and end points are the same!) but we shouldn't do that... therefore this trace, since it's the first on its subplot, should interpret 'tonexty'
as 'tozeroy'
.
var subplotAndType = trace.xaxis + trace.yaxis + trace.type; | ||
var firstScatter = fullLayout._firstScatter; | ||
if(!firstScatter[subplotAndType]) firstScatter[subplotAndType] = trace.uid; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, scatter_fill_corner_cases
top subplots were also prevented from filling to zero with 'tonexty'
because only one subplot could have the "first scatter" trace on it. This commit 🔪 gd.firstscatter
and replaces it with one trace (uid) per subplot, attached to fullLayout
. The stacked area mocks 🔒 this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. This is probably the most underrated piece in this PR. I always found that gd.firstscatter
less-than-ideal. This is a welcome improvement.
if(cd.length !== serieslen) { | ||
// TODO: verify this never happens and remove | ||
throw new Error('length mismatch!'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, missed this one... well, when I test ^^ I'll have plenty of confidence to remove it 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔪 in 8547cf8
// if we're stacking, "infer zero" gap mode gets markers in the | ||
// gap points - because we've inferred a zero there - but other | ||
// modes (currently "interpolate", later "interrupt" hopefully) | ||
// we don't draw generated markers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etpinard @nicolaskruchten do you agree with this choice? It only applies to points we generate in one trace to match the positions from another trace - those are the "gap points"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you agree with this choice?
+1 for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR!
I hope implementing those per-trace stack*
attributes wasn't too much of a headache. The two stackgaps
modes are looking great. 📈
Most of my comments are simply comments, with the exception of:
- I don't think we need that
alwaysSupplyDefaults
trace module category - mutating
gd.calcdata[i][j].i
isn't great. - is that hacky
fill
default logic really necessary? - what do you think adding a
'stack'
flag to scattermode
var subplotAndType = trace.xaxis + trace.yaxis + trace.type; | ||
var firstScatter = fullLayout._firstScatter; | ||
if(!firstScatter[subplotAndType]) firstScatter[subplotAndType] = trace.uid; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. This is probably the most underrated piece in this PR. I always found that gd.firstscatter
less-than-ideal. This is a welcome improvement.
// if we're stacking, "infer zero" gap mode gets markers in the | ||
// gap points - because we've inferred a zero there - but other | ||
// modes (currently "interpolate", later "interrupt" hopefully) | ||
// we don't draw generated markers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you agree with this choice?
+1 for me.
and remove error check added just for debugging during dev
So from a high-level API standpoint, why do we want |
It's unusual for sure, but I wouldn't want to rule it out. What if you have one stack series for data and another for fits? Then one stack would need its fill removed, since they're overlapping. Or prediction/extrapolation - these might not overlap but still you might want different styling for corresponding items in each stack. Or two back-to-back stacks, like those plots that have male on one side and female on the other, with the axis in the middle (we could manage that one with an analog of
But, perhaps both of these concerns could be assuaged by making a boolean |
OK, I'll buy the "back to back stacks" argument :) Could we make sure the documentation clearly explains whether or not stack normalization applies across or within subplots please? I don't know the answer but I'd like to and I think we should canonicalize it in the docs! |
I think we can live without an extra |
Within subplots (and within stack groups, if there are multiple on one subplot) -> 00d7d22 |
Down to 2️⃣ unresolved comments: |
Nicely done 💃 |
We held out a long time on this one, but stacked area charts are finally coming to plotly.js.
The API is as discussed in #1217:
stackgroup
attributes to somescatter
traces and they become a stack.stackgaps: 'interrupt'
. That's going to require some finicky drawing code, particularly if we want to support arbitraryline.shape
so I'll leave it for later. But'infer zero'
(default) and'interpolate'
are included here.In order to make it work well in various edge cases I made a number of preparatory changes:
Lib.sort
c87ccb3 wraps the built-inArray.sort
with a check for whether the array is already perfectly sorted (or perfectly reversed), that for arrays of length 1e5+ can be a 10x or better speedup for already-sorted arrays, and should have very little penalty for unsorted arrays. For stacked area I expect the vast majority of the time the data will already be sorted, so that's why I implemented this now and this is the only place I used it, but I bet there are other places it would be useful as well.axes_range_type
baseline change belongs in this commit but I put it in the autorange commit instead)cc @etpinard @antoinerg @nicolaskruchten