Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Poisson: Fix undefined behavior and support f64 output #795
Poisson: Fix undefined behavior and support f64 output #795
Changes from 1 commit
31aad14
c03e2c8
638b6be
eec9bba
90833ca
2553cb5
15b9a39
84d89a7
ec99801
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is not an optimal
f32
implementation and I'm not sure it's even an acceptable one (e.g. thea
parameter below rounds to1
). I would suggest just usingf64
arithmetic and converting at function start/end, though there's very little point to supportingPoisson<f32>
in this case (only really for the type deduction).Additionally, this function prototype is okay for internal use but not if we wished to expose
log_gamma
since we could not add correct implementations for other float types without specialization. A trait would be better, though clunky since we only have one method.Statrs's Gamma function implementations are likely better.
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.
It essentially evaluates a polynomial, so I think it should be fine, even if the coefficients have less precision. I thinks it is more likely that too many coefficients are used for the target precision.
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.
It might help for sampling from the uniform distribution.
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.
How does this help with the uniform distribution?
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.
We are doing some rejection sampling for the Poisson distribution, sampling
f32
instead off64
in the loop might help.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.
Ah, you mean speed up usage of uniform, not help implement uniform. Yes, and when we get a specialisation of
log_gamma
it may speed things up for any(?) situation wheref32
is faster thanf64
.