Skip to content
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

Improve GammaIPSampler and GammaMTSampler #1617

Merged
merged 8 commits into from
Sep 24, 2022
Merged

Conversation

devmotion
Copy link
Member

@devmotion devmotion commented Sep 13, 2022

This PR fixes the comment #1615 (comment).

This improves performance:

On the master branch

julia> using Distributions, BenchmarkTools

julia> @btime rand($(Gamma(0.6)));
  53.511 ns (0 allocations: 0 bytes)

julia> @btime rand($(Gamma(0.6)));
  53.992 ns (0 allocations: 0 bytes)

julia> @btime rand($(Gamma(0.6)), 1_000);
  50.164 μs (1 allocation: 7.94 KiB)

julia> @btime rand($(sampler(Gamma(0.6))), 1_000);
  49.876 μs (1 allocation: 7.94 KiB)

This PR

julia> using Distributions, BenchmarkTools

julia> @btime rand($(Gamma(0.6)));
  41.323 ns (0 allocations: 0 bytes)

julia> @btime rand($(Gamma(0.6)));
  41.254 ns (0 allocations: 0 bytes)

julia> @btime rand($(Gamma(0.6)), 1_000);
  32.739 μs (1 allocation: 7.94 KiB)

julia> @btime rand($(sampler(Gamma(0.6))), 1_000);
  31.606 μs (1 allocation: 7.94 KiB)

Edit:

The fixes of GammaIPSampler required a generalization of the GammaMTSampler. When updating it I discovered a bug in the current implementation: The factor in the squeeze function (used in the cheap acceptance check) was incorrect and hence unnecessarily often the more expensive acceptance check had to be evaluated. Fixing this issue leads to a significant performance improvement:

On the master branch

julia> using Distributions, BenchmarkTools

julia> @btime rand($(Distributions.GammaMTSampler(Gamma(1.0, 1.0))), 10_000);
  217.579 μs (2 allocations: 78.17 KiB)

julia> @btime rand($(Distributions.GammaGDSampler(Gamma(1.0, 1.0))), 10_000);
  372.066 μs (2 allocations: 78.17 KiB)

julia> @btime rand($(Distributions.GammaMTSampler(Gamma(2.0, 2.0))), 10_000);
  205.843 μs (2 allocations: 78.17 KiB)

julia> @btime rand($(Distributions.GammaGDSampler(Gamma(2.0, 2.0))), 10_000);
  308.139 μs (2 allocations: 78.17 KiB)

julia> @btime rand($(Distributions.GammaMTSampler(Gamma(100.0, 2.0))), 10_000);
  200.171 μs (2 allocations: 78.17 KiB)

julia> @btime rand($(Distributions.GammaGDSampler(Gamma(100.0, 2.0))), 10_000);
  145.451 μs (2 allocations: 78.17 KiB)

With this PR

julia> using Distributions, BenchmarkTools

julia> @btime rand($(Distributions.GammaMTSampler(Gamma(1.0, 1.0))), 10_000);
  141.777 μs (2 allocations: 78.17 KiB)

julia> @btime rand($(Distributions.GammaGDSampler(Gamma(1.0, 1.0))), 10_000);
  372.156 μs (2 allocations: 78.17 KiB)

julia> @btime rand($(Distributions.GammaMTSampler(Gamma(2.0, 2.0))), 10_000);
  138.271 μs (2 allocations: 78.17 KiB)

julia> @btime rand($(Distributions.GammaGDSampler(Gamma(2.0, 2.0))), 10_000);
  304.170 μs (2 allocations: 78.17 KiB)

julia> @btime rand($(Distributions.GammaMTSampler(Gamma(100.0, 2.0))), 10_000);
  127.696 μs (2 allocations: 78.17 KiB)

julia> @btime rand($(Distributions.GammaGDSampler(Gamma(100.0, 2.0))), 10_000);
  147.143 μs (2 allocations: 78.17 KiB)

@devmotion devmotion changed the title Improve GammaIPSampler Improve GammaIPSampler and GammaMTSampler Sep 13, 2022
GammaMTSampler(d, c, κ)

# We also pre-compute the factor in the squeeze function
return GammaMTSampler(promote(d, c, κ, 331//10_000)...)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the master branch, an unnecessarily large factor of 0.331 is used in the squeeze function (reference: https://dl.acm.org/doi/10.1145/358407.358414)..

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2022

Codecov Report

Base: 85.95% // Head: 85.94% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (8772805) compared to base (97f5ed0).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1617      +/-   ##
==========================================
- Coverage   85.95%   85.94%   -0.01%     
==========================================
  Files         129      129              
  Lines        8086     8098      +12     
==========================================
+ Hits         6950     6960      +10     
- Misses       1136     1138       +2     
Impacted Files Coverage Δ
src/multivariate/mvtdist.jl 62.74% <100.00%> (ø)
src/samplers/gamma.jl 97.02% <100.00%> (-2.98%) ⬇️
src/univariate/continuous/chisq.jl 85.36% <100.00%> (+2.03%) ⬆️
src/univariate/continuous/gamma.jl 94.50% <100.00%> (ø)
src/samplers/multinomial.jl 87.17% <0.00%> (+2.56%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

andreasnoack and others added 2 commits September 24, 2022 13:38
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
@devmotion devmotion merged commit 852af6a into master Sep 24, 2022
@devmotion devmotion deleted the dw/gammaipsampler branch September 24, 2022 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants