Improve algorithm for sampling `Beta` #1000

vks · 2020-07-17T15:42:53Z

Fixes #999.

cc @rasa200

saona-raimundo · 2020-07-17T17:28:18Z

Great!

Do you mind sharing the reference for the algorithm you are implementing here?

Edit:

It was part of your commits and the new documentation.

Reference:

R. C. H. Cheng (1978).
Generating beta variates with nonintegral shape parameters.
Communications of the ACM 21, 317-322.
https://doi.org/10.1145/359460.359482

Thank you!!

dhardy · 2020-07-17T19:03:34Z

That was quick @vks! Okay if I review some time this weekend? (Or is someone else wants to, I won't hog all the work 😉)

vks · 2020-07-18T12:16:44Z

@rasa200 I see you already found the source, sorry for not posting it in the PR description. The PDF is a bit tricky to access, but you could also look at the R source code for rbeta (which I personally avoided, because it is GPL code, and Rand's license is more liberal).

@dhardy Sure!

dhardy

Just starting to review, though I probably won't finish today.

@vks could you compare benchmarks before/after with a few parameter selections? Speed is not really a deciding factor in this case but it would be nice to have some idea how this performs.

rand_distr/src/gamma.rs

vks · 2020-07-19T14:56:20Z

It seems like the performance of this PR is actually worse, at least for the parameters I looked into:

On master:

test distr_beta_large_param_different      ... bench:     146,886 ns/iter (+/- 2,585) = 54 MB/s
test distr_beta_large_param_similar        ... bench:     145,855 ns/iter (+/- 980) = 54 MB/s
test distr_beta_mixed_param                ... bench:     248,922 ns/iter (+/- 3,614) = 32 MB/s
test distr_beta_small_param                ... bench:     357,560 ns/iter (+/- 4,676) = 22 MB/s

This PR:

test distr_beta_large_param_different      ... bench:     252,111 ns/iter (+/- 5,843) = 31 MB/s
test distr_beta_large_param_similar        ... bench:     268,847 ns/iter (+/- 23,299) = 29 MB/s
test distr_beta_mixed_param                ... bench:     282,934 ns/iter (+/- 46,543) = 28 MB/s
test distr_beta_small_param                ... bench:     350,247 ns/iter (+/- 40,186) = 22 MB/s

vks · 2020-07-30T11:37:40Z

I think we want this new algorithm for small parameters, but it might make sense to use the old algorithm for large parameters. I also did not really optimize the algorithm in this PR.

dhardy · 2020-07-30T14:06:55Z

Speed isn't a critical factor (though can be nice). Sorry that I didn't do the review yet.

vks · 2020-07-30T15:08:13Z

Speed isn't a critical factor (though can be nice).

Fair enough! However, the algorithm I implemented is actually two rejection-sampling algorithms: BC is used for min(alpha, beta) <= 1, BB is used for larger parameters. It would be unfortunate to switch to a less efficient algorithm for large parameters. However, the reference claims that BB is more efficient than sampling the gamma distribution twice (our current algorithm), so maybe something is off with my implementation/comparison (or maybe their results don't apply to modern hardware).

Of course, correctness is much more important, but I did not compare the algorithms for large parameters, so I don't really have an argument for using BB over the current implementation. 😅

Sorry that I didn't do the review yet.

No worries, this is not crucial or time-critical.

vks · 2020-08-04T18:02:28Z

I rebased on master, fixed the benchmarks (that were broken on master) and added them to CI (we did not test the rand_distr benchmarks, so it was not noticed that they were broken).

The last two commits could be merged separately and are arguably more urgent than the rest of this PR.

dhardy · 2020-08-28T15:29:07Z

Sorry that I still didn't get to this. @rasa200 would you care to do a review? The main point is to have a second person check correctness (or at least conformance to the paper).

This will have to target rand_distr v0.4, which will hopefully come out soon after rand v0.8.

saona-raimundo · 2020-08-29T11:09:38Z

Yes! I will try my best!

saona-raimundo

The algorithmic part was super clear and followed the paper line-by-line. In doing so, a typo was reproduced (there is a number that should be 72 instead of 18, see comments).

The pre-simulation constants should be corrected. By making the choice of setting the parameters a and b from the beginning, before the choice of the variant BB or BC, and the introduction of the variable switched_params, there is a disparity with the formulas presented in the paper. In the paper, they set the variables a and b as the min and max respectively for the BB algorithm, but they do the other way around for the BC algorithm!! Making confusion in the formulas when coding! (You can check this in the first line of each algorihtm in the paper).

rand_distr/src/gamma.rs

vks · 2020-09-06T23:04:32Z

Thanks for the detailed review! I overlooked that the definition for a and b was different in the two algorithms. To fix this, I decided to switch a and b in algorithm BC, so that it corresponds more closely to the reference. I also fixed the wrong switching condition.

I don't think there is a typo in the paper, as far as I can see, the division by for was absorbed in the numerator, so the denominator should not be affected.

This should be faster than the gamma variate transformation we are currently using, and it seems to work better for parameters smaller than one. The algorithm is also used by the R language, however I did not consult their implementation in order to avoid licensing problems. Reference: R. C. H. Cheng (1978). Generating beta variates with nonintegral shape parameters. Communications of the ACM 21, 317-322. https://doi.org/10.1145/359460.359482

vks · 2020-09-06T23:18:16Z

Now that the algorithm is fixed, we are doing much better with performance:

This PR:

test distr_beta_large_param_different      ... bench:      66,837 ns/iter (+/- 3,437) = 119 MB/s
test distr_beta_large_param_similar        ... bench:      73,134 ns/iter (+/- 7,125) = 109 MB/s
test distr_beta_mixed_param                ... bench:      95,295 ns/iter (+/- 7,092) = 83 MB/s
test distr_beta_small_param                ... bench:      93,504 ns/iter (+/- 5,298) = 85 MB/s

master:

test distr_beta_large_param_different      ... bench:     146,886 ns/iter (+/- 2,585) = 54 MB/s
test distr_beta_large_param_similar        ... bench:     145,855 ns/iter (+/- 980) = 54 MB/s
test distr_beta_mixed_param                ... bench:     248,922 ns/iter (+/- 3,614) = 32 MB/s
test distr_beta_small_param                ... bench:     357,560 ns/iter (+/- 4,676) = 22 MB/s

saona-raimundo · 2020-09-07T08:29:43Z

Great!! Thank you very much!!

I agree with all the changes, and sorry about the "typo" of the constants, it was my own mistake: it is right the way it is in the code and the paper.

dhardy · 2020-09-07T09:41:14Z

rand_distr/CHANGELOG.md

@@ -4,6 +4,9 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

+## [Unreleased]
+- Improve algorithm for sampling `Beta` (#1000)


Could this be a little more specific?

I changed it to "New Beta sampling algorithm for improved performance and accuracy".

dhardy

Are you both happy with the code now?

saona-raimundo · 2020-09-07T11:58:16Z

I am happy with the code :)

vks · 2020-09-07T15:55:27Z

@dhardy I'm happy with the code, too. Could you please approve the PR?

dhardy

Sure thing. Thanks for the review, Raimundo.

vks mentioned this pull request Jul 17, 2020

Unexpected sample values from beta distribution for small parameters #999

Closed

dhardy reviewed Jul 19, 2020

View reviewed changes

rand_distr/src/gamma.rs Outdated Show resolved Hide resolved

saona-raimundo mentioned this pull request Aug 4, 2020

Beta Distribution values wrong for a=b---> 0 pytorch/pytorch#15738

Open

vks force-pushed the improve-beta branch from 3ca8bc2 to 3d8cebb Compare August 4, 2020 18:00

vks mentioned this pull request Aug 5, 2020

rand_distr 0.3 tracker #921

Closed

3 tasks

saona-raimundo suggested changes Sep 6, 2020

View reviewed changes

vks force-pushed the improve-beta branch from 4d7dd68 to a0ddbed Compare September 6, 2020 23:05

vks added 9 commits September 7, 2020 01:06

Replace constants with more precise values

bd4c59e

Optimize struct size of Beta

296dabd

rand_distr: Fix benchmarks

b6b241b

Add rand_distr benches to CI

e114a13

Fix CI script

bf12368

Fix Beta sampling algorithm

90eca16

Reduce code duplication

1320262

Fix value stability tests

c080f19

vks force-pushed the improve-beta branch from a0ddbed to c080f19 Compare September 6, 2020 23:07

Fix changelog

c85e736

dhardy reviewed Sep 7, 2020

View reviewed changes

More specific changelog

769e5d1

dhardy approved these changes Sep 8, 2020

View reviewed changes

vks merged commit 7a1f51c into rust-random:master Sep 8, 2020

vks deleted the improve-beta branch September 8, 2020 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve algorithm for sampling `Beta` #1000

Improve algorithm for sampling `Beta` #1000

vks commented Jul 17, 2020

saona-raimundo commented Jul 17, 2020 •

edited

Loading

dhardy commented Jul 17, 2020 •

edited

Loading

vks commented Jul 18, 2020

dhardy left a comment

vks commented Jul 19, 2020

vks commented Jul 30, 2020

dhardy commented Jul 30, 2020

vks commented Jul 30, 2020 •

edited

Loading

vks commented Aug 4, 2020

dhardy commented Aug 28, 2020 •

edited

Loading

saona-raimundo commented Aug 29, 2020

saona-raimundo left a comment

vks commented Sep 6, 2020 •

edited

Loading

vks commented Sep 6, 2020

saona-raimundo commented Sep 7, 2020

dhardy Sep 7, 2020

vks Sep 7, 2020

dhardy left a comment

saona-raimundo commented Sep 7, 2020

vks commented Sep 7, 2020

dhardy left a comment •

edited

Loading

Improve algorithm for sampling Beta #1000

Improve algorithm for sampling Beta #1000

Conversation

vks commented Jul 17, 2020

saona-raimundo commented Jul 17, 2020 • edited Loading

dhardy commented Jul 17, 2020 • edited Loading

vks commented Jul 18, 2020

dhardy left a comment

Choose a reason for hiding this comment

vks commented Jul 19, 2020

vks commented Jul 30, 2020

dhardy commented Jul 30, 2020

vks commented Jul 30, 2020 • edited Loading

vks commented Aug 4, 2020

dhardy commented Aug 28, 2020 • edited Loading

saona-raimundo commented Aug 29, 2020

saona-raimundo left a comment

Choose a reason for hiding this comment

vks commented Sep 6, 2020 • edited Loading

vks commented Sep 6, 2020

saona-raimundo commented Sep 7, 2020

dhardy Sep 7, 2020

Choose a reason for hiding this comment

vks Sep 7, 2020

Choose a reason for hiding this comment

dhardy left a comment

Choose a reason for hiding this comment

saona-raimundo commented Sep 7, 2020

vks commented Sep 7, 2020

dhardy left a comment • edited Loading

Choose a reason for hiding this comment

Improve algorithm for sampling `Beta` #1000

Improve algorithm for sampling `Beta` #1000

saona-raimundo commented Jul 17, 2020 •

edited

Loading

dhardy commented Jul 17, 2020 •

edited

Loading

vks commented Jul 30, 2020 •

edited

Loading

dhardy commented Aug 28, 2020 •

edited

Loading

vks commented Sep 6, 2020 •

edited

Loading

dhardy left a comment •

edited

Loading