Fix Chi pdf for x < 0 #1498
Fix Chi pdf for x < 0 #1498
Conversation
test/chi.jl
Outdated
| @test isfinite(mean(Chi(1000))) | ||
|
|
||
| # see #1497 | ||
| @test pdf(Chi(3), -.5) == 0.0 |
There was a problem hiding this comment.
| @test pdf(Chi(3), -.5) == 0.0 | |
| @test pdf(Chi(3), -0.5) == 0.0 |
src/univariate/continuous/chi.jl
Outdated
|
|
||
| logpdf(d::Chi, x::Real) = (ν = d.ν; | ||
| function logpdf(d::Chi, x::Real) | ||
| x < 0 && return 0.0 |
There was a problem hiding this comment.
This is type unstable, better to compute the result as much as possible (as done for other distributions).
| x < 0 && return 0.0 |
There was a problem hiding this comment.
BTW one should also not return 0 but -Inf (of the correct type).
There was a problem hiding this comment.
Ah sorry, was thinking of pdf.
rikhuijzer
left a comment
rikhuijzer
left a comment
There was a problem hiding this comment.
Also much better to use multi line function here indeed instead of the one-liner with bracket
Note that I don't have merge rights here, but more eyes is more better I guess
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
|
Thanks for the detailed in-line changes. I merged them and confirmed they work locally. I think this can be merged! |
Codecov Report
@@ Coverage Diff @@
## master #1498 +/- ##
==========================================
- Coverage 84.46% 84.26% -0.20%
==========================================
Files 124 124
Lines 7520 7514 -6
==========================================
- Hits 6352 6332 -20
- Misses 1168 1182 +14
Continue to review full report at Codecov.
|
|
Unfortunately, it seems to break downstream tests of Tracker in DistributionsAD: e.g., https://github.com/JuliaStats/Distributions.jl/runs/5009289497?check_suite_focus=true#step:6:322 We have to check if this is a downstream problem or an actual issue with this PR. |
|
Apart from that the PR looks good 🙂 |
|
Seems to be an issue in Tracker since it does not define derivatives for LogExpFunctions yet. It's an easy fix, I'll open a PR. Let's hold back with merging this PR to avoid breaking downstream packages and rerun tests (and merge if they pass) once a new version of Tracker is released. |
|
The downstream issues are fixed 🙂 Thanks a lot for the PR @pdeffebach! |
4 participants
Closes #1497