-
-
Notifications
You must be signed in to change notification settings - Fork 12
Avoid returning NaN from Gamma::sample #46
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
base: master
Are you sure you want to change the base?
Conversation
82ec7a8 to
4aca53d
Compare
|
I think our general approach to deal with extreme parameters should be to just return an Error from |
4aca53d to
cf3f46b
Compare
I'm not certain what a good threshold / measurement of accuracy would be for Gamma in particular, and for now have added a generic warning about inaccuracy to the documentation.
|
dhardy
left a comment
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.
@benjamin-lieser handling edge cases like infinity and underflow-to-zero in a manner which is consistent with the general distribution is often simpler to deal with and good enough — so in my opinion this is the more sensible approach to take in general.
src/gamma.rs
Outdated
| /// When the shape (`k`) or scale (`θ`) parameters are close to the upper limits | ||
| /// of the floating point type `F`, the implementation may overflow and | ||
| /// produce `inf`; similarly, if either is sufficiently close to `0.0`, | ||
| /// it may output `0.0`. Sampling may become inaccurate if `k` is close | ||
| /// to zero and `θ` is very large. |
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.
Could this be a little more specific (e.g. as noted in your recent comment @mstoeckl)?
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.
Yes, I've briefly explained the underflow behavior. On looking at the actual samples, I see the distribution is actually sampled pretty well with shape = 1/200, scale=1.0. The actual distribution is < the minimum positive f64 with notable (>0.02) probability, and the Gamma sampler correctly returns 0.0 with about the expected frequency. The distr_test failures result from the Kolmogorov-Smirnov implementation in distr_test assuming real values, not accounting for floating point discretization, and treating the jump from 0.0 to the min f64 as an actual discrepancy.
This changes the order of multiplications used to compute the result to avoid multiplying zero with an expression that can overflow to +inf. Note that the parameter combinations which could lead to this (shape very close to zero, scale very close to the max float value) continue to be inaccurately handled; the Gamma distribution sampler will now tend to return zero instead of NaN for them. The limit (shape 0, scale inf) is not well defined.
cf3f46b to
a3bb2b5
Compare
CHANGELOG.mdentrySummary
Gamma::sample could return
NaNdue to multiplying0.0with something that had overflowed; this PR makes the sampler return 0.0 in such cases instead. This changes the order of some multiplications, which can slightly change the output of the sampler on typical inputs.Motivation
This is another edge case that I found found with a parameter fuzzer (https://github.com/mstoeckl/rand_distr/commits/fuzz-params/), that turns out to be avoidable with only slight additional complexity.
Details
The specific NaN that I noticed came from multiplying
u.powf(self.inv_shape), which for smallinv_shapecould be zero, with the output of GammaLargeShape::sample, which could overflow to+infifscalewas large. The general method to avoid this is to delay applying the distribution scale factor until the end of the calculation, and before that point try to operate on values which are close in magnitude to 1.I also added a special case to Gamma::new to handle shape, scale = +inf in particular.
The parameter combinations which would previously produce Nan (shape very close to zero, scale very close to the max float value) will continue to produce somewhat inaccurate output (producing more zero output values than expected, because
u.powf(self.inv_shape)is likely to underflow whenshapeis small) Fixing this particular edge case would likely require rewriting the GammaSmallShape sampler to use a different and possibly more expensive sampling method.