-
Notifications
You must be signed in to change notification settings - Fork 421
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
Use a faster implementation of AliasTables #1848
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1848 +/- ##
==========================================
+ Coverage 85.94% 85.96% +0.02%
==========================================
Files 144 144
Lines 8656 8647 -9
==========================================
- Hits 7439 7433 -6
+ Misses 1217 1214 -3 ☔ View full report in Codecov by Sentry. |
Given the performance improvements, shouldn't this be used in or even contributed to StatsBase? |
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.
Looks good to me, that's a great performance improvement!
Co-authored-by: David Widmann <[email protected]>
AliasTables.jl provides a high performance, high precision alias table implementation. This PR switches from the use of StatsBase implementation details to the public API of the AliasTables package. After making that switch, I also re-tuned the sampling threshold for
Multinomial
so that it selects the right algorithm more appropriately.Benchmarks using Chairmarks.jl ("a => b" means I got the result "a" when running on master and the result "b" when running on PR branch):
Multinomial sampling (some multinomial changes backed out of this PR, see #1848 (comment), #1834. The multinomial bnechamrks here are as of 1c3530c)
AliasTable sampling (same benchmarks as #1831)
AliasTable construction
However, these constructor time comparisons are not apples to apples. The old ones give the wrong answer without normalization (see #832) while the new ones do not require normalization (fixes #832).
The new version is slow largely because of unreasonably strict precision guarantees in the normalization code. I can add an option (which could be on by default for Distributions.jl) for faster and less precise construction. However, I haven not added this lower precision option because I doubt construction time is typically a limiting factor in AliasTable sampling speed (one would have to generate the weights at a rate of less than 20ns/weight for that to be an issue)
The new version is also faster (and more precise) for integer inputs (not including the old runtimes because the old algorithm produced incorrect answers)
The sampling assembly is now branch-less and shorter
Before
After
For how this speedup is possible, see https://aliastables.lilithhafner.com/dev/#Implementation-details.