Re: [boost] [random] new threefry random engine

Subject: Re: [boost] [random] new threefry random engine
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2014-04-18 00:54:01

AMDG

On 04/17/2014 04:19 PM, Thijs van den Berg wrote:
> I've completed a nice new random engine that I would like to propose for addition to boost/random.
> 
> The engine is called "threefry" and it's based on the paper "Parallel random numbers: as easy as 1, 2, 3" by Salmon, John K. and Moraes, Mark A. and Dror, Ron O. and Shaw, David E. 
> 
> I've been using it myself a lot for the last couple of years, and now I've rewritten it so that it will fit in the boost random framework and adhere to the concepts. It has some really nice properties, in particular for large scale parallel computing:
> * High quality randomness (according to the BigCrush tests) with a cycle length of 2^258
> * Fast (comparable to Mersenne Twister) & little memory usage (13x 64 bit integers)
> * O(1) discard and guarantees for non-overlapping sequences when the (up to 256 bit) seeds are unequal
> 
> I've uploaded the code, description and tests to https://github.com/sitmo/threefry
> The actual engine is at https://github.com/sitmo/threefry/blob/master/random/include/boost/random/threefry.hpp
> 
> I would really appreciate any feedback about the idea for inclusion and of course comments about the quality of the code,tests,docs
> 

I /really/ dislike the constraint that
rounds shall be 13 or 20.  It looks like
encrypt_counter follows a regular pattern.
I'd strongly prefer it to be implemented
as a loop and allow rounds to be any
non-negative integer.  Not all possible
values make sense, but that's normal.
You have to know what you're doing when
you define a specialization of any random
number engine.

Do not use reinterpret_cast in operator().
The behavior is undefined, and will certainly
differ depending on endianness.

I'm not sure that I like the fact that the
textual representation interleaves the three
arrays.  Also output is a function of key and
counter and does not need to be stored.  (The
same observation applies to the comparison
operators)

Is it really a good idea to expose set_key/set_counter?

It would be better to have an explicit width parameter,
like mersenne_twister instead of relying on the size of
UIntType.  I'd also prefer to see uint_least64_t in place
of uint64_t.  (Actually I'd like it even more if the
arrays stored UIntType, but this doesn't look particularly
feasible, given the way the algorithm works.)

Don't put usings in namespace boost.  The other engines
have using declarations for backwards compatibility,
which is not a concern for a new engine.

Run inspect (http://www.boost.org/tools/inspect).  There's
at least one use of max that needs to be protected.

In Christ,
Steven Watanabe