Handle truncation endpoints equal to support endpoints #1730
Conversation
Consider `truncated(Normal(), -Inf, Inf)`. Currently, this returns a
`Truncated`-wrapped `Normal`. However, truncating a normal distribution
to ±∞ is... not actually truncating it at all. We handle this case when
the bounds are specified as `nothing` and return the untruncated
distribution, but not when one or both bounds are equal to or beyond
the respective bounds of the support. If we recode `lower <= minimum(d)`
and/or `upper >= maximum(d)` to `nothing`, we're able to avoid a
`Truncated` wrapper in more cases. And when we do make a `Truncated`,
equivalent truncations can now compare equal and hit the same code paths
for dispatch on the truncation using `nothing` as appropriate.
Two things to note with this implementation:
- `truncated(d::D, a, b)` is no longer type stable since it can now
return either `D` or `Truncated{D}` whereas before it only returned
`D` if `a === b === nothing`.
- Nonsensical truncation, e.g. `truncated(Poisson(), -10, -1)`, still
works and is still nonsensical.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1730 +/- ##
==========================================
- Coverage 85.90% 85.90% -0.01%
==========================================
Files 142 142
Lines 8566 8575 +9
==========================================
+ Hits 7359 7366 +7
- Misses 1207 1209 +2
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I can see why this behaviour might be convenient (and this approach actually came up in some discussion in a private downstream repo not too long ago). But currently I think that
truncated(d::D, a, b) is no longer typestable
is a major drawback, and that the increased convenience does not justify making truncated type unstable. In particular, since users can easily avoid these problems by specifying correct bounds or nothing (or, IMO more conveniently, the keyword argument syntax).
Consider
truncated(Normal(), -Inf, Inf). Currently, this returns aTruncated-wrappedNormal. However, truncating a normal distribution to ±∞ is... not actually truncating it at all. We handle this case when the bounds are specified asnothingand return the untruncated distribution, but not when one or both bounds are equal to or beyond the respective bounds of the support. If we recodelower <= minimum(d)and/orupper >= maximum(d)tonothing, we're able to avoid aTruncatedwrapper in more cases. And when we do make aTruncated, equivalent truncations can now compare equal and hit the same code paths for dispatch on the truncation usingnothingas appropriate.Two things to note with this implementation:
truncated(d::D, a, b)is no longer type stable since it can now return either aDor aTruncated{D}whereas before it only returned aDifa === b === nothing.truncated(Poisson(), -10, -1), still works and is still nonsensical.