-
Notifications
You must be signed in to change notification settings - Fork 421
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
Throw an error when truncating outside of support #1723
base: master
Are you sure you want to change the base?
Conversation
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 think for AD backends such as Zygote we should allow to disable (and skip automatically) the check by uaing the @check_args
macro - in other distributions standard checks (without @check_args
) impacted performance severely.
Looks like this approach doesn't quite do it, as evidenced by the large number of test failures 🙃 |
Some of them at least are due to tests that have to be fixed/removed: https://github.com/JuliaStats/Distributions.jl/actions/runs/5017513840/jobs/8995725428?pr=1723#step:6:67 Some others seem to indicate that julia> d = truncated(Normal(0, 1), 100, 115)
Truncated(Normal{Float64}(μ=0.0, σ=1.0); lower=100.0, upper=115.0)
julia> mean(d)
100.00999800099926
julia> var(d)
9.994005086296622e-5
julia> pdf(d, 101)
NaN
julia> logpdf(d, 101)
Inf
julia> d.logtp
-Inf
julia> d.tp
0.0 I think we might be able to fix these existing numerical issues for normal distributions (at least) by improving the numerical accuracy in the calculation of julia> SpecialFunctions.logerf(100/sqrt(2), 115/sqrt(2)) - log(2)
-5005.5242086942035 It seems one part could consist of improving julia> logdiffcdf(Normal(0, 1), 115, 100)
-Inf And then maybe |
I opened #1728 to address the |
4b19063
to
9c3bf42
Compare
Co-authored-by: David Widmann <[email protected]>
Something @devmotion wrote in #1721 reminds me that there's actually 2 separate issues that are closely related, but might require different solutions.
I would argue that something like Now the question is, should we handle these cases differently? It seems to me like this pull request would error out on julia> logdiffcdf(Normal(0,1), 0, 0)
-Inf Note that this currently works "as proper", in my opinion. julia> t = truncated(Normal(0,1), 0, 0)
Truncated(Normal{Float64}(μ=0.0, σ=1.0); lower=0.0, upper=0.0)
julia> rand(t)
0.0
julia> t = truncated(Normal(0,1), 1, 1)
Truncated(Normal{Float64}(μ=0.0, σ=1.0); lower=1.0, upper=1.0)
julia> rand(t)
1.0 |
You're right, the |
Fixes #843 using option 2 as described here. The implementation checks for 0 probability within the truncated region but continues to allow coincident truncation bounds if they're within the support. That avoids breaking cases like that shown in #1712.