-
Notifications
You must be signed in to change notification settings - Fork 433
consistently treat Bernoulli as a distribution on Bools #2022
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2022 +/- ##
=======================================
Coverage 86.36% 86.36%
=======================================
Files 146 146
Lines 8789 8789
=======================================
Hits 7591 7591
Misses 1198 1198 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| function quantile(d::Bernoulli{T}, p::Real) where T<:Real | ||
| 0 <= p <= 1 ? (p <= failprob(d) ? zero(T) : one(T)) : T(NaN) | ||
| function quantile(d::Bernoulli, p::Real) |
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.
Can you revert the changes to quantile and cquantile? The current API (probably historically based on Rmath) is to never error but to return NaN for invalid arguments. I think this should be changed, but that requires a more general change of the API of these functions: #1805
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.
reverting quantile implies also reverting median for consistency, and then basically we lose the point of this PR
any hope of #1805 in the foreseeable future?
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.
btw, quantile throws sometimes already:
julia> quantile(Bernoulli{Int}(1), 2)
ERROR: InexactError: Int64(NaN)
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.
any hope of #1805 in the foreseeable future?
Has been ready since > 2 years, just needs a review.
reverting quantile implies also reverting median for consistency, and then basically we lose the point of this PR
The current implementation of quantile is wrong (IMO) but it's consistent. It would be worse if some distributions would behave differently, we can't start erroring just for Bernoulli. You can still change mode, modes and support without touching quantile/cquantile.
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.
we can't start erroring just for Bernoulli
It already errors sometimes (and with a different exception). In addition to the example above:
quantile(Bernoulli(1//3), 2)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 would be worse if some distributions would behave differently, we can't start erroring just for Bernoulli
Actually, I don't understand why you say "throw error in quantile()" = "behave differently". It's actually the common way distributions behave in this package:
julia> quantile(Binomial(10, 0.2), 2)
ERROR: InexactError: Int64(NaN)
julia> quantile(Categorical([0.2, 0.8]), 2)
# the error-throwing code has an issue, but clearly intends to throw DomainError:
ERROR: MethodError: no method matching DomainError()
julia> quantile(DiscreteUniform(1, 5), 2)
# it would better throw than return 10:
10
julia> quantile(DiscreteNonParametric([1,2,5], [0.1, 0.2, 0.7]), 2)
# the error-throwing code has an issue, but clearly intends to throw DomainError:
ERROR: MethodError: no method matching DomainError()Apparently, this PR not just makes Bernoulli internally consistent, but also consistent with other distributions. Wasn't my explicit intention, but happy it turned out this way :)
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.
These are all bugs 😅 The current API is to return NaN for invalid inputs.
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.
These are all bugs 😅 The current API is to return NaN for invalid inputs.
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.
Any references to that? Where do you get that "The current API is to return NaN for invalid inputs"?
Currently, the situation is that basically all discrete distributions return integers from quantile. And naturally, they throw an exception when p not in 0..1.
Documentation doesn't seem to contradict this in the slightest.
Even Bernoulli often behaves like that. It is Bernoulli{Float}() that is an exception (resolved by this PR).
Currently, it's quite inconsistent: eg,
eltype === Bool,rand()::Bool,minimum()::Bool, butmodeandmedianareInt, andquantileisT(often, float).With this PR
BernoullireturnsBoolwhenever possible and makes sense.This is both self-consistent between functions on
Bernoulli, and also makes it consistent with other discrete distributions likeBinomial/Categorical/DiscreteUniform/DiscreteNonParametricthat return the minimal type (oftenInt) for theirquantileand other functions.