Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upSwitch Random to use the PCG RNG #778
Conversation
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
process-bot
Dec 9, 2016
Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!
Here is what to expect next, and if anyone wants to comment, keep these things in mind.
process-bot
commented
Dec 9, 2016
|
Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it! Here is what to expect next, and if anyone wants to comment, keep these things in mind. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
|
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
THIS IS SO RAD |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
evancz
Dec 12, 2016
Member
This PR is really great, thank you for doing all this! I need to do some planning about when to release various things, but I just wanted to comment to say that things are looking good!
|
This PR is really great, thank you for doing all this! I need to do some planning about when to release various things, but I just wanted to comment to say that things are looking good! |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
rtfeldman
Apr 21, 2017
Member
@evancz I'd love to see this get the same treatment as https://github.com/elm-lang/core/pull/850
|
@evancz I'd love to see this get the same treatment as https://github.com/elm-lang/core/pull/850 |
mgold
referenced this pull request
Jun 25, 2017
Closed
Problem with small values in Random.int on init #635
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
evancz
Jul 8, 2017
Member
I read through this today and merged it into dev here elm-lang@48e640e
@mgold, I notice you use (|>) in a bunch of bitwise operations. Do you mind switching this code to not use that operator, instead just saying Bitwise.and x y like normal? It would be annoying to have a performance regression because we were depending on a couple optimizations happening in the right order, so I'd just feel more comfortable if we did not use anything extra.
That said, thanks again for your excellent work on this! Excited to see how it goes when we are testing out 0.19 :)
|
I read through this today and merged it into dev here elm-lang@48e640e @mgold, I notice you use That said, thanks again for your excellent work on this! Excited to see how it goes when we are testing out 0.19 :) |
evancz
closed this
Jul 8, 2017
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
mgold
Jul 8, 2017
Contributor
Pipe removal in #883.
Thanks so much for merging! You are quite welcome.
|
Pipe removal in #883. Thanks so much for merging! You are quite welcome. |
mgold commentedDec 9, 2016
•
edited
Edited 1 time
-
mgold
edited Dec 9, 2016 (most recent)
As we've discussed, this PR switches out the implementation of the random number generator while not altering the public API at all. It uses the PCG generator.
You can see my benchmarking code here, but you have two questions:
Is it fast?
Yes.
This is Chrome on Mac, although I ran it on Safari and Firefox and saw similar results. They can vary a bit between runs; I've seen speedup factors as low as 1.2x and higher than 10x, but 6-7x is common. Although the "how much" part is somewhat finicky, it definitely is faster. (Looks like bitwise op inlining paid off!)
Also note that integer generation is faster for ranges that are a power of two (e.g. 0-4095) because a faster algorithm can be used. There is no equivalent for floats, and the tests bear that out.
Is it random?
It is demonstrably more random than what we have now.
I've tested the statistical properties of PCG in the past and it has always out-performed the old/current implementation. However, I had to rewrite the code to produce the random numbers for 0.18, and I think I wound up with fewer random bits before node ran out of memory. However, I can definitely state that this patch passes the first of the dieharder test battery, while core fails. The author describes the test as follows:
This implementation is much less sensitive to the choice of random seed for the first value. Contrast the current implementation:
This shows that all initial seeds less than 53 thousand (plus or minus) will generate
Truefor their first boolean. True, you shouldn't use the random library like this, but timestamps are similarly distributed in a continuous range that is a small subset of 32-bit integers. So... we kind of are using the random library like this.So: it's faster, it's more random, a seed is only one integer, and it uses fewer magic numbers. Sound good?