-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add 'cumulative' histogram 'mode' for CDF #1189
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
Changes from 4 commits
62aeefb
9c0dea0
fd7526b
17c04c5
4bc28dd
c12b7cf
4d02af7
53b61aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,6 +71,46 @@ module.exports = { | |
].join(' ') | ||
}, | ||
|
||
cumulative: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's fine. Thanks for the info! |
||
valType: 'boolean', | ||
dflt: false, | ||
role: 'info', | ||
description: [ | ||
'If true, display the cumulative distribution by summing the', | ||
'binned values. Use the `direction` and `centralbin` attributes', | ||
'to tune the accumulation method.' | ||
].join(' ') | ||
}, | ||
|
||
direction: { | ||
valType: 'enumerated', | ||
values: ['increasing', 'decreasing'], | ||
dflt: 'increasing', | ||
role: 'info', | ||
description: [ | ||
'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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. fixed in 4d02af7 |
||
].join(' ') | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thoughts on the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok with
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? |
||
|
||
currentbin: { | ||
valType: 'enumerated', | ||
values: ['include', 'exclude', 'half'], | ||
dflt: 'include', | ||
role: 'info', | ||
description: [ | ||
'Only applies if `cumulative=true.', | ||
'Sets whether the current bin is included, excluded, or has half', | ||
'of its value included in the current cumulative value.', | ||
'*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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for putting that option in! The name This has me thinking: maybe we should group all cumulative: {
enabled: true || false,
direction: 'increasing' || 'decreasing',
currentbin: 'include' || 'exclude' || 'half'
} By @chriddyp's #1189 (comment) where we might extend There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My only concern about this is that 90% of users won't use anything but There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about letting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nested in 4d02af7 |
||
].join(' ') | ||
}, | ||
|
||
autobinx: { | ||
valType: 'boolean', | ||
dflt: null, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -246,5 +246,76 @@ describe('Test histogram', function() { | |
]); | ||
}); | ||
|
||
describe('cumulative distribution functions', function() { | ||
var base = {x: [1, 2, 3, 4, 2, 3, 4, 3, 4, 4]}; | ||
|
||
it('makes the right base histogram', function() { | ||
var baseOut = _calc(base); | ||
expect(baseOut).toEqual([ | ||
{b: 0, p: 1, s: 1}, | ||
{b: 0, p: 2, s: 2}, | ||
{b: 0, p: 3, s: 3}, | ||
{b: 0, p: 4, s: 4}, | ||
]); | ||
}); | ||
|
||
var CDFs = [ | ||
{p: [1, 2, 3, 4], s: [1, 3, 6, 10]}, | ||
{ | ||
direction: 'decreasing', | ||
p: [1, 2, 3, 4], s: [10, 9, 7, 4] | ||
}, | ||
{ | ||
currentbin: 'exclude', | ||
p: [2, 3, 4, 5], s: [1, 3, 6, 10] | ||
}, | ||
{ | ||
direction: 'decreasing', currentbin: 'exclude', | ||
p: [0, 1, 2, 3], s: [10, 9, 7, 4] | ||
}, | ||
{ | ||
currentbin: 'half', | ||
p: [1, 2, 3, 4, 5], s: [0.5, 2, 4.5, 8, 10] | ||
}, | ||
{ | ||
direction: 'decreasing', currentbin: 'half', | ||
p: [0, 1, 2, 3, 4], s: [10, 9.5, 8, 5.5, 2] | ||
}, | ||
{ | ||
direction: 'decreasing', currentbin: 'half', histnorm: 'percent', | ||
p: [0, 1, 2, 3, 4], s: [100, 95, 80, 55, 20] | ||
}, | ||
{ | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. It might be nice to test There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I don't think there's anything special to do based on Thoughts on any of this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree 100% here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
]; | ||
|
||
CDFs.forEach(function(CDF) { | ||
var direction = CDF.direction, | ||
currentbin = CDF.currentbin, | ||
histnorm = CDF.histnorm, | ||
p = CDF.p, | ||
s = CDF.s; | ||
|
||
it('handles direction=' + direction + ', currentbin=' + currentbin + ', histnorm=' + histnorm, function() { | ||
var traceIn = Lib.extendFlat({}, base, { | ||
cumulative: true, | ||
direction: direction, | ||
currentbin: currentbin, | ||
histnorm: histnorm | ||
}); | ||
var out = _calc(traceIn); | ||
|
||
expect(out.length).toBe(p.length); | ||
out.forEach(function(outi, i) { | ||
expect(outi.p).toBe(p[i]); | ||
expect(outi.s).toBeCloseTo(s[i], 6); | ||
expect(outi.b).toBe(0); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
||
}); | ||
}); |
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.