-
Notifications
You must be signed in to change notification settings - Fork 430
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
Uniform sampling: use Canon's method #1287
Conversation
Based on canon-uniform-benches branch, revised
Also: add "unbiased" feature flag
This is a small tweak unsupported by evidence, but brings SIMD in line with unbiased integer range sampling.
Allows simpler tests
Note: unbiased does pass current value-stability tests, but could fail extra ones in the future.
Baseline results (new benchmark on master)
New results (compared to baseline)
New results (unbiased feature)
...I hate micro-benchmarking (see results in #1286). Looks like we should just use Lemire's method for distribution sampling in all cases. |
Agreed, especially if it is less biased. |
Bench re-runs (lower clock speed, better formatted): results.ods Highlights:
So, yes, this supports the idea that we should always use Lemire's method for distribution sampling. |
Remaining question: whether to keep both biased and unbiased options for single-sampling (using a feature flag). See #494. I am inclined to keep this under the following conditions:
There is not a strong rationale for this however, we could reduce to just one implementation (either). |
I'm inclined to merge this as-is. Review please, maybe @TheIronBorn or @vks? |
Thanks @TheIronBorn. Updated. |
I'd like to merge this but am still waiting for a reviewer to approve (policy requires review is not by the author). @TheIronBorn you last reviewed this; would you mind revisiting? |
Closes #570, #1145, #1154, #1196, #1286. See also #1172 (TODO: SIMD), #494 (here we add "unbiased" feature flag).
Also implements
PartialEq
for all ourUniform
impls andEq
for all but FP. See #1217.Yet another PR to finally update
Uniform
integer sampling (maybe):