-
-
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
Add 'cumulative' histogram 'mode' for CDF #1189
Conversation
Here's a proof-of-concept PR made to attract more attention from our plotly attribute associates @chriddyp @cldougl but also @alexcjohnson @rreusser @monfera In this PoC, a |
@@ -112,6 +113,8 @@ module.exports = function calc(gd, trace) { | |||
// average and/or normalize the data, if needed | |||
if(doavg) total = doAvg(size, counts); | |||
if(normfunc) normfunc(size, total, inc); | |||
if(trace.mode === 'cumulative') cdf(size); |
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.
Maybe 'cumulative'
should have a different meaning for histnorm !== ''
and histfunc !== 'count'
?
@@ -71,6 +71,16 @@ module.exports = { | |||
].join(' ') | |||
}, | |||
|
|||
mode: { |
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.
or maybe alternatively, a boolean cumulative
attribute could do the trick. I don't see any other possible modes for histograms.
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.
This is a cool addition--I think I'd prefer a cumulative
boolean attribute. That's pretty explicit and +1 about not seeing any other possible modes.
Also, not sure if this is being too nitpicky across traces, but I'm not super convinced that this use case provides a good parallel to scatter mode
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.
⏫ the winning argument so far
Yes, something like |
I like the sound of |
Love this! How do you imagine this relating to cumulative distributions graphed as a line chart? That's how I'm used to seeing CDFs and PDFs. Lots of nice examples on the plotly feed: https://plot.ly/feed/?q=CDF Something similar that comes up pretty frequently is visualizing the aggregations that we provide in These aggregations come up a lot and CDFs and PDFs seem to also fall into this family. Originally I thought that we would provide this type of functionality through an aggregation transform + a scatter chart but maybe it could just be a different rendering option in the traces themselves, like adding Would love @alexcjohnson 's feedback on ^^ too |
@chriddyp Re: line mode for histograms - yes, I think it's a good idea, but it would be good to get real line/area trace stacking working first. Then it would be easy to plumb this into a rolling average transform if someone wants that. Agreed that it would be misleading to do this on bars. @etpinard re: cumulative histograms - I haven't looked at the attributes you've proposed yet, but we should be careful about our presentation. The way you've done it in the gif up top - each bar is at the same location as its PDF analog, with height equal to its own height plus the sum of all bars before it - is very common (it's used in the wikipedia article you linked), but it's also arguably wrong. Visually you've shifted the distribution half a bar to the left. Imagine thinking of the bars as a continuous function (ie constant over the domain of each bar) and integrating that function, which is really what the CDF is supposed to mean. You'd actually get a piecewise linear result where the value at the right edge of each bar is the sum of that bar and all bars before it. I suppose if you really want to keep bars, you could imagine each cumulative bar being the sum of all the bars before it plus half of the current bar... that would be more "correct" but that seems like it would just confuse people. Or you can show both the previous total and the current bin: In short though, I really don't like bars for CDFs, however common they are. Take that with a grain of salt though, I haven't used them much for real data analysis myself, would love to hear the perspective of someone who has. There's something similar to be said about the plot @chriddyp posted (which has now disappeared? But I think I remember what it looked like) although ironically with a partially opposite solution. It looks like in that plot you're showing the exact CDF by adding a data point for each individual sample? In that case linearly interpolating between points is incorrect, the CDF does not linearly increase from one sample to the next, it jumps up exactly at each sample - because it's really an integral of delta functions, one for each sample. So in plotly.js language, you should use Alternatively (and arguably more correctly in terms of the visual significance of the point markers, but confusing for the same reasons as above) you could set the vertical position of each point to the number of samples to the left of this one plus half a sample for the current one, and connect points using Some people also normalize to This kind of situation is, incidentally, exactly what's hard about doing real stacked area charts correctly... you'd be trying to stack y values of functions that are defined at uneven x values... so for the second trace, do you make steps at the x values of the first trace, even though the second trace doesn't have a data point there? |
avoid an empty bin at the start. Tested via histogram_test
'increases from left to right. If *decreasing* we sum later bins', | ||
'so the fresult decreases from left to right.' | ||
].join(' ') | ||
}, |
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.
direction='decreasing'
is to invert the accumulation - ie if you want "how much of the distribution is past this point" instead of "how much is before this point" (although note that it's not exactly the sum minus the increasing CDF unless you choose currentbin='exclude'
for one, or currentbin='half'
for both (see below).
Thoughts on the name direction
? I'm not super excited about it but it seems OK.
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.
I'm ok with direction
here. But we should keep in mind that direction
is now present in several attribute containers. Currently:
- In pie traces,
direction: 'clockwise' || 'counterclockwise'
- In updatemenus,
direction: 'left' || 'right' || 'up' || 'down'
- In the animtion config,
direction: 'forward' || 'reverse'
- In base layout for polar plots:
direction: 'clockwise' || 'counterclockwise'
I suppose it would be nice to make enumerated attributes of the same name share the same posibile values when used in different containers. Or maybe that's too much to ask for?
'*include* is the default for compatibility with various other', | ||
'tools, however it introduces a half-bin bias to the results.', | ||
'*exclude* makes the opposite half-bin bias, and *half* removes', | ||
'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.
OK, maybe nobody will use this option, but I put it in to satisfy my own frustration with the visual flaw in the common practice (#1189 (comment)). Is it clear enough what the options mean?
And not to beat a dead horse, but as well as fixing the position bias I think 'half'
also does a better job of representing the width of the distribution. To take the extreme case, lets say you have a histogram with all the samples in a single bin. Although they could be all at exactly the same value, generally they aren't. But the standard way to display this would have the CDF going from zero to max instantaneously, as a step function, which implies no width at all to the distribution. 'half'
on the other hand would rise in 2 steps - which visually implies a width of 1 bin.
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.
Thanks for putting that option in!
The name currentbin
bothers me a little bit because it doesn't sound associated with cumulative
.
This has me thinking: maybe we should group all cumulative
attributes into a nested object.
cumulative: {
enabled: true || false,
direction: 'increasing' || 'decreasing',
currentbin: 'include' || 'exclude' || 'half'
}
By @chriddyp's #1189 (comment) where we might extend cumulative: true
to other trace types down the road, adding a cumulative
nested object to scatter would be less intrusive I think.
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.
This has me thinking: maybe we should group all cumulative attributes into a nested object.
My only concern about this is that 90% of users won't use anything but enabled
, and cumulative: true
is easier than cumulative: {enabled: true}
. But it would disambiguate direction
too... so maybe it's worthwhile.
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.
What if
{
type: 'histogram',
x: [/* */],
cumulative: true
}
expanded to
{
type: 'histogram',
x: [/* */],
cumulative: {
enabled: true,
direction: 'increasing',
currentbin: 'include'
}
}
in _fullData
?
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.
I thought about letting cumulative: true
expand to cumulative: {enabled: true}
internally, but I don't think it's a good idea - it would make it very confusing for folks to switch to the full form if they start with the simple one. I think I'll just change it to the nested structure.
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.
nested in 4d02af7
and fix its tests for the improved behavior
{ | ||
currentbin: 'exclude', histnorm: 'probability', | ||
p: [2, 3, 4, 5], s: [0.1, 0.3, 0.6, 1] | ||
} |
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.
It might be nice to test cumulative: true
with other histfunc
and histnorm
settings.
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.
Another good call by @etpinard 🌮 (and see also #1189 (comment))
What should we do with cumulative enabled and histnorm='density'
or 'probability density'
? As the code stands, CDFs using 'density'
would rise to N/binSize
(# samples / width of each bin) and 'probability density'
would rise to 1/binSize
. That seems useless and confusing, so I'd propose to interpret "cumulative" to mean an integral in these cases, ie 'density'
would rise to N
and 'probability density'
would rise to 1
, which then means in CDF mode these are equivalent to histnorm=''
and 'probability'
respectively.
I don't think there's anything special to do based on histfunc
- some of these would also give strange results, but then the user is clearly asking for something strange.
Thoughts on any of 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.
I don't think there's anything special to do based on histfunc - some of these would also give strange results, but then the user is clearly asking for something strange.
I can see this being used in time series CDFs. Think payments over time: bin
by date
and then cumulatively sum
by payment amount
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.
I'd propose to interpret "cumulative" to mean an integral in these cases, ie 'density' would rise to N and 'probability density' would rise to 1
I agree 100% here.
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.
I can see this being used in time series CDFs. Think payments over time: bin by date and then cumulatively sum by payment amount
Absolutely - and that will work just fine without modification (tests to come). I was just saying I don't think there's anything that needs altering based on histfunc
, like what I'm planning to do for histnorm
.
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.
'Only applies if `cumulative=true.', | ||
'If *increasing* (default) we sum all prior bins, so the result', | ||
'increases from left to right. If *decreasing* we sum later bins', | ||
'so the fresult decreases from left to right.' |
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.
result typo
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.
fixed in 4d02af7
@@ -1296,7 +1296,7 @@ function _restyle(gd, aobj, _traces) { | |||
'tilt', 'tiltaxis', 'depth', 'direction', 'rotation', 'pull', | |||
'line.showscale', 'line.cauto', 'line.autocolorscale', 'line.reversescale', | |||
'marker.line.showscale', 'marker.line.cauto', 'marker.line.autocolorscale', 'marker.line.reversescale', | |||
'xcalendar', 'ycalendar' | |||
'xcalendar', 'ycalendar', 'cumulative', 'currentbin' |
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.
probably need direction
in here too.
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.
... unless it's already part of that list.
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.
yep, look 3 lines up. That is a bit of a concern... that now all the different direction
uses are coupled to each other. Though I'm assuming this whole list is not long for this world, and within some reasonable timeframe we'll delegate all of this to the trace modules.
@@ -71,6 +71,46 @@ module.exports = { | |||
].join(' ') | |||
}, | |||
|
|||
cumulative: { |
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.
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.
test image in 53b61aa
One thing this showed is that we need a way to harmonize autobins across traces, and that it needs to know about cumulative
. To make this example work I needed to manually extend the bin range for the smaller trace, otherwise its CDF ended too soon. Actually, CDFs never end, really... so perhaps the even better thing to do would be to look at the axis range and draw bins out to the edge. Anyway, fixing this will be a bigger project so I'll make an issue for it rather than try to address it here.
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.
Anyway, fixing this will be a bigger project so I'll make an issue for it rather than try to address it here.
That's fine. Thanks for the info!
}, | ||
{ | ||
// behaves the same as without *density* | ||
direction: 'decreasing', currentbin: 'half', histnorm: 'density', |
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.
Looking good!
💃 Thanks for taking this one home! |
resolves #1180