-
Notifications
You must be signed in to change notification settings - Fork 431
More efficient PoissonBinomial
#1285
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
Conversation
| new{T}(p, pb) | ||
| mutable struct PoissonBinomial{T<:Real,P<:AbstractVector{T}} <: DiscreteUnivariateDistribution | ||
| p::P | ||
| pmf::Union{Nothing,Vector{T}} # lazy computation of the probability mass function |
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.
Did you consider using an empty vector here? That would allow the struct to be immutable.
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 did. I wanted to try this approach first since arguably it is a bit "clearer" (nothing clearly indicates that it was not initialized whereas otherwise one has to check isempty) and the alternative would require to change poissonbinomial_pdf to an in-place function (which is clearly possible though). But I have no strong opinion here, I can compare it to the other approach.
Codecov Report
@@ Coverage Diff @@
## master #1285 +/- ##
==========================================
- Coverage 81.60% 81.54% -0.07%
==========================================
Files 117 115 -2
Lines 6665 6636 -29
==========================================
- Hits 5439 5411 -28
+ Misses 1226 1225 -1
Continue to review full report at Codecov.
|
|
I compared the implementation in this PR with an implementation of an immutable struct that uses Without
|
|
Thanks for benchmarking this. The compiler optimizations for the small union here might be really good. Why would e.g. the |
Yes, IMO this was the most surprising result. I assume that we just see the allocation of the empty vector (80 bytes) in the benchmark without |
|
This is puzzling to me julia> @btime PoissonBinomial($p);
90.943 ns (1 allocation: 32 bytes)
...
julia> @btime mean(PoissonBinomial($p));
104.703 ns (0 allocations: 0 bytes)I'd think that the latter would time construction as well as the mean calculation whereas the first would only time the construction. Why is the first allocating when the second one isn't? |
|
Yes, this was surprising to me as well. One can get rid of the allocation by changing the returned value: julia> f(p) = (PoissonBinomial(p); return)
julia> @btime f($p);
88.468 ns (0 allocations: 0 bytes)I just don't fully understand why one gets this additional allocation. I inspected the output of in the version with one allocation. So I assume in the non-allocating case (and also the |
|
How should we move on with this PR? |
|
I think we should just stick with your current implementation since the alternative one didn't show improvements. |
For many computations, such as e.g.
meanandvar, it is not necessary to evaluate the probability mass function ofPoissonBinomialfor all possible outcomes. However, currently, it is computed whenPoissonBinomialis constructed. For large number of trials this induces a significant delay and might even make it too inefficient to work withPoissonBinomialat all.In this PR, instead the values of the probability mass function are only computed when a user performs an evaluation that requires it.
Simple benchmark on my computer:
master
This PR
This PR is motivated by TuringLang/MCMCChains.jl#263: often it is sufficient to evaluate the mean and the variance (as an uncertainty measure) of the Poisson binomial distribution of the statistic of interest, and therefor the quadratic complexity of the evaluation of the probability mass function slows down the computations for large number of trials (e.g., MCMC samples in this case) unnecessarily.