search

JuliaStats / Distributions.jl

A Julia package for probability distributions and associated functions.
Other
1.1k stars 414 forks source link

`0 <= cdf(Normal(),x) <= 1` not always true. #1853

Closed lrnv closed 5 months ago

lrnv commented 5 months ago

I had to clamp here https://github.com/lrnv/Copulas.jl/pull/197 to fix https://discourse.julialang.org/t/copulas-jl-turing-jl-numerical-stability/113243/3

Is this normal behavior ? By that i mean, should it be considered a bug from Distributions.jl, or should i simply enforce it on my end ? This second option is probably the easiest but not the most interesting, since others might end up on the same issue.

devmotion commented 5 months ago

No, it should always be between 0 and 1 for non-NaN inputs. Do you have a failing input? (BTW the Turing examples in the discourse thread could be improved a bit; instead of TruncatedNormal(...., Inf) I'd recommend using truncated(Normal(...); lower=...) and I'd try to avoid Turing.@addlogprob! as much as possible since it circumvents a few Turing internals and hence can lead to problems and unexpected results).

devmotion commented 5 months ago

BTW for Normal, the cdf function just uses normcdf in StatsFuns (https://github.com/JuliaStats/StatsFuns.jl/blob/5e866baa763ff0f5d279185c1923e1cd87ea5699/src/distrs/norm.jl#L45-L53) which is based on erfc in SpecialFunctions, so if there is a problem then likely it's a problem in SpecialFunctions.

lrnv commented 5 months ago

The discourse example fails with cdf being greater than one on a particular sample (or smaller than zero on others), from, indeed, the SpecialFunctions... but it's almost impossible to me to track the call stack to understand what is the failling value inputed by Turing (maybe someone with a bit more debugging skills could).

So for Float64, SpecialFunctions.erfc calls into the standard C library libm. I think we cannot go lower...

I think I saw a ForwardDiff.Dual in the call stack, maybe this points to somewhere else ?

So as a conclusion maybe we should close this, and I will keep the clamping on my side. Since it should always be a no-op, I do not care that much.

lrnv commented 5 months ago

After a bit of massaging of the MWE, let me close this to open another one.