-
Notifications
You must be signed in to change notification settings - Fork 25
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
Binomial Sampler #90
Binomial Sampler #90
Conversation
That would be great addition! There's implementation using condensed tables algorithms but it requires costly setup. Also there's inverse incomplete beta in P.S. I'll check whether CI still works. |
It does not :-( |
I've added some benchmarks but I get
So it would seem that my method is x50 slower. But in https://github.com/idontgetoutmuch/mwc-random/blob/master/System/Random/MWC/Binomial.hs I can run
So 6.8 seconds via the table method vs
So 0.05 seconds for the TPE (triangle parallelogram exponential) method making it about x10 faster. I don't know what benchmarking actually measures but maybe you @Shimuuar can explain what is going on? |
I fixed CI and added PAPI-based benchmarks. It turns out they are quite useful for making a lot of measurements quickly I'm going to look into what current benchmarks actually measure. |
I ran some benchmarks. If I use
and
then the TPE method is faster but if I use
and
then the table method is faster. I am guessing the table is calculated once in this case and cached somehow but not if
|
I tried running these (which is why I needed a faster binomial sampler):
The TPE method is about x100 faster so I am still baffled as to what the benchmarks are measuring. |
I've looked into benchmarks and have no idea what they're measure for condensed table benchmarks either. They should measure either sample generation time or table setup time (which is expensive). They measure some mix of two. There's no other way. I'll have to rewrite them |
Yes. Current benchmarks in |
I am putting my scribbles here in case I need them. I am cleaning up the PR so it can be merged without "squeezing". I can do that later and measure performance improvements. I am following the gsl implementation https://git.savannah.gnu.org/cgit/gsl.git/tree/randist/binomial_tpe.c.
|
@Shimuuar this can be further improved but can be merged - maybe I should add some tests (not that the other distributions have any). I didn't want to do too much more until you had bottomed out the benchmarking. Are tables really faster? They are according to the benchmarks but not when I use that method in a program. |
I'm going to fix benchmarks tomorrow. Tables have rather peculiar performance characteristic: sampling from table should be fast. AFAIR on the order of single P.S. I haven't looked at PR in details yet |
@Shimuuar ready for review :-) |
Great! I'm still trying to fix benchmarks. |
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.
Now that's proper impenetrable numerical codes :) It will take some time for me to understand what's going on
System/Random/MWC/Distributions.hs
Outdated
-> g -- ^ Generator | ||
-> m Int | ||
{-# INLINE binomial #-} | ||
binomial nTrials prob gen = |
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.
What is semantics of p<0 || p>1
? There are two choices: throw error as gamma
and chiSquare
do or to clamp in [0,1] range (and deal with NaN separately)
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.
I'll follow the convention and throw an error. I can't see why anyone would want a NaN propagated. Conceivably we could have a fast version with no checking?
System/Random/MWC/Distributions.hs
Outdated
-> m Int | ||
{-# INLINE binomial #-} | ||
binomial nTrials prob gen = | ||
if prob == 0.0 |
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.
Purely stylistically. Could you turn nested ifs into guards?
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 done :-)
System/Random/MWC/Distributions.hs
Outdated
|
||
-- Acceptance / rejection comparison | ||
step5 :: Int -> Double -> m Int | ||
step5 ix v = if var <= accept |
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.
And here as well
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.
Done :-)
@@ -1,7 +1,7 @@ | |||
cabal-version: 3.0 | |||
build-type: Simple | |||
name: mwc-random | |||
version: 0.15.0.2 |
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.
That would be minor bump to 0.15.1.0 not patch bump. We're adding new 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.
Done
I am hope I am not "teaching grandmothers to suck eggs" but it is a rejection sampling method with an empirical proposal distribution (which I believe they prove to have the correct support in a reference which seems can only be obtained by paying a lot of money). They divide up the region into the triangular segment, two parallelogram segments and two segments covered by the exponential distribution. I might try and draw the picture and put it in the docs. The triangular region is below the target distribution so if you are there then you always accept. You then decide where you are in the parallel or exponential regions and do an accept / reject (depending on whether you are above or below the target - sorry for being obvious here). Further they do something they call squeezing (to improve the acceptance rate?) which I haven't investigated yet but will be a future PR. HTH. |
I hope that is now clear: if you are in area "One" then you are below the target so you always accept. For areas "Two", "Three" and "Four" you have to decide whether you are above or below and then reject or not. See the original description which I think has an error in the majorizing and minorizing functions which is luckily benign except when you try to recreate the diagram. |
Sorry I was terribly busy lately and only got to skip paper and PR. My plan is to write better test for sampler: using likelihood ratio check that we sample from correct distribution and if everything goes well just merge PR and do further documentation improvements/microptimizations separately |
Should this be in the repo to recreate the figure? Probably not but it would be nice to put it somewhere other than in a PR comment.
|
Test fails for n_trials >≈ 25
I wrote property test (branch |
That rather suggests the implementation:
I will go and take a look. |
I did a chi-squared test by hand for the values that quick check gave and agree that we can reject the null hypothesis :-( |
I just tried a reference implementation in C and the chi squared checks out so I must have transcribed something incorrectly. I will look tomorrow at my code. |
|
@Shimuuar I think it is now ready to merge but I have 2 questions:
|
Thanks!
Also I forgot about program that draws diagram. If you want to add to repository program which draws diagram for algorithm just drop it into (nonexitent) |
@Shimuuar I don't understand why we get these bounds warnings but of course with the suggested bounds, some of the builds fail. |
91ee327
to
a7ee5a0
Compare
Thank you! This PR could be merged. I'm going to make few documentation tweaks and some optimization work. But that could be done separately |
I am in the middle of implementing Kachitvichyanukul, V. and Schmeiser, B. W. (1988) but the inverse method (in the current PR) could be a first step. What do you think?