You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR simplifies the (log)(c)cdf implementations and fixes the evaluation with -Inf, Inf, and NaN.
There are some problems with the current implementation:
It does not handle NaN consistently: e.g.
julia>cdf(Normal(), NaN)
NaN
julia>cdf(Exponential(), NaN)
0.0
julia>cdf(TriangularDist(0, 1), NaN)
1.0
It defines cdf(d::DiscreteUnivariateDistribution, x::Real) = cdf(d, floor(Int, x)) etc and a default implementation for cdf(::DiscreteUnivariateDistribution, x::Integer). This is problematic since
one has to define both cdf(d, x::Real) and cdf(d, x::Integer) for e.g. discrete LocationScale, Truncated and MixtureModel and DiscreteNonParametric to avoid method ambiguity errors
not all discrete distributions have integer-valued support but still a cdf for integers is defined that might not make sense or in the worst case silently produce incorrect results, e.g.
(since logcdf(Dirac(3.4), 3.5) is forwarded to logcdf(Dirac(3.4), 3) which then calls log(cdf(Dirac(3.4, 3))) = log(0) = -Inf). These bugs are difficult to discover and to avoid when implementing a discrete distribution (I fixed some of these for DiscreteNonParametric in Generalize LocationScale to discrete distributions #1286).
it is impossible to evaluate cdf(d, -Inf), cdf(d, Inf), or `cdf(d, NaN) with these definitions: e.g.
For Poisson this can be fixed by generalizing the StatsFuns macro to inputs of type Real but this is still an issue for other native implementations such as Categorical for which the same errors are thrown. The evaluation of cdf(d, -Inf) or cdf(d, Inf) is useful e.g. in the construction of truncated distributions - it allows to remove the current type heuristics (not included in this PR).
the default calls cdf_int(d, x) which assumes integer-valued support but not inputs of type Int
cdf_int(d, x) handles Inf, -Inf, and NaN and calls cdf(d, floor(Int, x)) for other values (not implemented!)
for distributions with integer-valued support one has to implement cdf(d, ::Int) and gets support for real values including Inf, -Inf, and NaN for free
for distributions with non-integer-valued support one has to implement cdf(d, ::Real) but not cdf(d, ::Int) (and there exists no incorrect default implementation)
there are no silent incorrect results since an error is thrown if one does not implement cdf(d, ::Int) explicitly
one can call integerunitrange_cdf etc. instead of implementing cdf(d, ::Int) from scratch if the distribution has a unitrange of integers as support (currently the default implementation of cdf(d, ::Int))
this also removes method ambiguity issues and the implementation of Truncated, LocationScale and MixtureModel can be simplified.
generalizing the StatsFuns macro and also defining cdf(d, x::Real) etc. instead of cdf(d, x::Int) for discrete distributions
Note: The simplified implementation for Truncated requires JuliaStats/LogExpFunctions.jl#21, thus tests will fail until the LogExpFunctions PR is released.
Hmm I'm not sure. Hopefully I could convey the main ideas in the comment above and then I guess it depends on which points you want to examine or discuss more carefully. I assume that src/univariates.jl should show the main structure. The changes in src/truncate.jl, src/univariate/locationscale.jl and src/mixtures/... show how it simplifies (and fixes) the existing implementation. And finally the changes in src/univariate/... fix and simplify different (log)(c)cdf implementations. I extended the tests and (log)(c)cdf are tested for all modified distributions (I only changed these distributions since tests failed), so I am quite confident that the changes are correct. But it's always better if someone else checks it as well 🙂
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR simplifies the
(log)(c)cdfimplementations and fixes the evaluation with-Inf,Inf, andNaN.There are some problems with the current implementation:
NaNconsistently: e.g.cdf(d::DiscreteUnivariateDistribution, x::Real) = cdf(d, floor(Int, x))etc and a default implementation forcdf(::DiscreteUnivariateDistribution, x::Integer). This is problematic sincecdf(d, x::Real)andcdf(d, x::Integer)for e.g. discreteLocationScale,TruncatedandMixtureModelandDiscreteNonParametricto avoid method ambiguity errorscdffor integers is defined that might not make sense or in the worst case silently produce incorrect results, e.g.logcdf(Dirac(3.4), 3.5)is forwarded tologcdf(Dirac(3.4), 3)which then callslog(cdf(Dirac(3.4, 3))) = log(0) = -Inf). These bugs are difficult to discover and to avoid when implementing a discrete distribution (I fixed some of these forDiscreteNonParametricin GeneralizeLocationScaleto discrete distributions #1286).cdf(d, -Inf),cdf(d, Inf), or `cdf(d, NaN) with these definitions: e.g.Poissonthis can be fixed by generalizing the StatsFuns macro to inputs of typeRealbut this is still an issue for other native implementations such asCategoricalfor which the same errors are thrown. The evaluation ofcdf(d, -Inf)orcdf(d, Inf)is useful e.g. in the construction of truncated distributions - it allows to remove the current type heuristics (not included in this PR).The PR fixes these problems by
cdf(d::DiscreteUnivariateDistribution, x::Real)but notcdf(::DiscreteUnivariateDistribution, x::Integer), similar to [Breaking] Fix inconsistent fallback behaviour of logpdf and pdf #1171cdf_int(d, x)which assumes integer-valued support but not inputs of typeIntcdf_int(d, x)handlesInf,-Inf, andNaNand callscdf(d, floor(Int, x))for other values (not implemented!)cdf(d, ::Int)and gets support for real values includingInf,-Inf, andNaNfor freecdf(d, ::Real)but notcdf(d, ::Int)(and there exists no incorrect default implementation)cdf(d, ::Int)explicitlyintegerunitrange_cdfetc. instead of implementingcdf(d, ::Int)from scratch if the distribution has a unitrange of integers as support (currently the default implementation ofcdf(d, ::Int))Truncated,LocationScaleandMixtureModelcan be simplified.cdf(d, x::Real)etc. instead ofcdf(d, x::Int)for discrete distributionsNaN,Inf, and-Infcorrectly and can deal with non-integer inputs (hopefully soon there will be many more native implementations: Use julia implementations for pdfs and some cdf-like functions StatsFuns.jl#113)(log)(c)cdfof many univariate distributions