-
Notifications
You must be signed in to change notification settings - Fork 3
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
Alternative clamping for NegativeBinomialMeanClust
#237
Conversation
Hey @damonbayer ! Thanks for the contribution! The underlying parametrisation we're using is determined by the variance-to-mean relationship: And I got taught the Your commit looks good and is maths-equivalent so just waiting on the CI. |
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 you check the failure of the unit test here?
This could just be because the committed approach is better/auto-safe!
@@ -33,11 +33,8 @@ function NegativeBinomialMeanClust(μ, α) | |||
if isnan(μ) || isnan(α) |
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.
Out of interest, does nextfloat
etc mean we can get rid of the logic branching here? That would be good from the pov of compilable tape for reverse diff.
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.
Is your desire to replace the DiscreteUniform(0, 1_000_000)
with a very wide NegativeBinomial
? I'm not sure of the right way to do that, but the clamp
method will not play nice with NaN
s.
❤️ I think prior to merging we should add @damonbayer to the authors list (
Longer term we should have some kind of contributing policy for this but above practice is my usual default so I think we should follow until we have said policy in place. |
Actually, it seems that the existing approach is safer. For the values in the test, the julia> big_mu = 1e30
alpha = 0.5
α = alpha
μ = big_mu
r = clamp(1 / α, nextfloat(zero(α)), prevfloat(typemax(α)))
p = clamp(1 / (1 + α * μ), nextfloat(zero(μ)), one(μ))
rand(NegativeBinomial(r, p))
ERROR: InexactError: trunc(Int64, 1.3967586116300517e30) Maybe it's just better to keep as is. |
Just noting I am really surprised that this is the case (and slightly sad). Not entirely clear to me what action we should take here? Promote to an issue for investigation or is there anything more to be done here? |
I have a related, longstanding issue in Distributions.jl JuliaStats/Distributions.jl#1512 |
So burrowing into this; the place that chucks an error is the call to e.g. big_mu = 1e30
alpha = 0.5
p = 1 / (1 + alpha + big_mu)
r = 1 / alpha
nb = NegativeBinomial(r, p)
rand(nb)
# InexactError: trunc(Int64, 1.0211481865897263e30) This is because
Poisson sampling branches into to a bunch of special cases for efficiency, in particular
This algo requires finding a value L = floor(Int, μ - 1.1484) And its |
Note that logpdf(nb, 1000)
# -129.86005643920763 |
So the options are here:
|
Another more radical possibility is that we accept that having models that can sample > |
So for this see my comment in the issue and I think generally wee should move discussion there. Nice investigation.
This is my preference I think and for now we work around? |
For my read of this is that we plan not to merge so closing. @damonbayer it would be a joy to have another contribution from you :) |
As requested, porting the clamping from https://github.com/damonbayer/immunity_semi_parametric_model/blob/d54162e1bda24950efdb5ce60da686cc47e2b36b/src/immunity_semi_parametric_model.jl#L9.
Some of the math in the existing code seemed a bit odd to me, so I made some simplifications:
_μ^2 / ex_σ² = _μ^2 / (_α * _μ^2) = 1 / _α
Perhaps I am missing something.