Several Improvements to the random package #3135
Conversation
|
|
||
| fun apply(h: TestHelper) => | ||
| let xoroshiro128 = XorOshiro128StarStar(5489) | ||
| h.assert_eq[U64](xoroshiro128.next(), 529225608228480) |
There was a problem hiding this comment.
Would it be possible (and would you consider it worth doing) to instead iterate over a list of U64 instead of repeating this line over and over?
There was a problem hiding this comment.
Also, are these test vectors from the papers, or from some other implementation? I suppose adding a comment where the numbers come from could be useful in the future.
There was a problem hiding this comment.
The numbers are from executions of the reference implementation in C. I added comments about each source.
I also initially wanted to use a list to iterate over, but i decided against it, as the effort would have been roughly the same and the current version has the advantage that the error message in case of failure will tell you at which position it failed, without counting lines while iterating through a list.
There was a problem hiding this comment.
Thanks! You're right about that line no detail, what you've chosen is better! =)
| DoNotOptimise[U64](x) | ||
| DoNotOptimise.observe() | ||
|
|
||
| class iso RandomBenchmark[T: Random ref] is MicroBenchmark |
packages/random/xorshift.pony
Outdated
| @@ -10,6 +10,15 @@ class XorShift128Plus is Random | |||
| var _x: U64 | |||
| var _y: U64 | |||
|
|
|||
| new from_64bit(x: U64 = 5489) => | |||
There was a problem hiding this comment.
Maybe from_u64 would be more consistent with other APIs?
Same comment applies to other additions.
|
@mfelsche should this be squashed? should it be merged? it's unclear to me. i'll leave it to for you to merge/squash as needed. |
|
Sorry, I was off for a little while. I am gonna add manual changelog entries and squash while providing a proper commit message. |
instead of floating-point multiplication. But only use it if the platform supports native 128 bit ops, as we need to multiply with U128, because we cannot (yet) usesmaller types when generating e.g. U32 with Random.int. Waiting for specialized generic functions here. Also added some benchmarks to show the performance increase. http://www.pcg-random.org/posts/bounded-rands.html states it is just as biased as fp-mult version and a quick local test, generating a histogram showed proper uniform distribution. Also added 'int_unbiased a slower but unbiased function for generating bounded random numbers.
xoroshiro128** is a new xoroshiro variant with less bias in the low bits than xoroshiro128** but also with a small performance impact. Also use new parameters for xoroshiro128+ as described in the NOTE here: http://xoshiro.di.unimi.it/xoroshiro128plus.c splitmix64 is a PRNG which only needs 64 bits of state and is primarily used to seed xoroshiro and xorshift generators from 64 bits only using the new 'from_64bits' constructors on those. splitmix64 should only be used when 64 bit of state is a hard requirement, otherwise use xoroshiro128+ or xoroshiro128**.
mfelsche
force-pushed
the
random-int-range
branch
from
04fa65e
to
63c5237
Compare
I packaged those changes in 1 PR but left them as separate commits. If any of the changes is controversial or should be split, i am happy to do so.
from_64bitsconstructor to 128 bit state PRNGs that uses SplitMix64 as suggested here: http://xoshiro.di.unimi.it/Random.int. But fall back to slower floating-point multiplication technique if no native 128 bit ops are supported.Random.int_unbiasedfunction for generating an unbiased bounded random number. For details see: http://www.pcg-random.org/posts/bounded-rands.html This one is slightly slower asRandom.int.Example benchmark run on my machine:
Legend:
random/intis the new fixed-point inversion method.random/int-fp-multis the oldRandom.intimplementation.