Fix rand for truncated normal with 0 variance
#1721
Conversation
src/truncated/normal.jl
Outdated
| if a <= μ <= b | ||
| z = 0.0 | ||
| else | ||
| return NaN |
There was a problem hiding this comment.
I think we would want to catch this case earlier in truncated: A distribution with zero mass is by definition not a distribution. See #843.
There was a problem hiding this comment.
Maybe let's throw an error here? I don't think returning NaN is particularly useful.
There was a problem hiding this comment.
I've opened #1723 as a means of catching this case earlier, but I'd also be fine throwing an error here.
|
Seems I was beaten to the punch by about 2 years: #1273 🙂 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1721 +/- ##
=======================================
Coverage 85.99% 85.99%
=======================================
Files 144 144
Lines 8666 8670 +4
=======================================
+ Hits 7452 7456 +4
Misses 1214 1214 ☔ View full report in Codecov by Sentry. |
|
This should also resolve #1867 |
Fixes issues 1712 and 1867.
ararslan
force-pushed
the
aa/degenerate-tn
branch
from
b488b3c to
5c79db1
Compare
| z = randnt(rng, a, b, d.tp) | ||
| if iszero(σ) | ||
| if a <= μ <= b | ||
| z = 0.0 |
There was a problem hiding this comment.
I assume this will cause type instabilities, e.g., if parameters of Normal and Truncated are Float32?
There was a problem hiding this comment.
z is always Float64, both here and the branch that calls randnt (that function requires all inputs be Float64 and it returns a Float64). But if µ and the truncation points are wider, say BigFloat, then you'll get a MethodError from randnt when isfinite(µ) && σ > 0 and a BigFloat result otherwise. (That behavior predates this PR.) Float32 works because extrema promotes the truncation points to Float64, so every branch will return Float64.
Fixes #1712
Fixes #1867