Issue 18715 - Non-documented unittests should not use unpredictableSeed or default Random alias
merged-on-behalf-of: Nathan Sashihara <n8sh@users.noreply.github.com>
The fix for 18631 broke code. Ranges cannot be marked with const or
inout in generic code. The range API does not require that any functions
be const or inout. In this case, with the changes, choice requires that
length and opSlice be const, breaking any code that uses a range with
choice that does not have a const length or opSlice (which is going to
be a large percentage of ranges with those functions which aren't
arrays).
XorshiftEngine.min is defined as 0 regardless of template parameters
but an XorshiftEngine cannot produce a value of zero if its internal
state has the same number of bits as the output element type.
Random choice default argument was deciding the type of the
template. I'm not sure if this is a deeper bug, but it can easily be
fixed with this patch which simply overloads the function.
Also added a unittest which would have failed under the old code.
As in XorshiftEngine, sanitizing the seed is more useful than throwing
an exception. The original check was broken because it checked the seed
before taking the modulus. Making this change allows unpredictableSeed
to be marked nothrow and @nogc.
As with the earlier test of range-based seeding for the 32-bit Mt19937,
this test is added purely in order to ensure consistency of behaviour in
future. It does not appear to match the behaviour of the superficially
similar sequence-based seeding available in the C++11 <random> version
of the Mersenne Twister.
It does however match the behaviour of the `MersenneTwisterEngine`
implementation available in `hap.random`, which was derived from the
`Boost.Random` implementation, but whose range-based seeding was copied
from Phobos. This would suggest that any divergence with C++11 is down
entirely to the seeding mechanism.
The word size for `MersenneTwisterEngine` is determined by the template
parameter `w`, not `UIntType`. For example, a generator with identical
parameters to the standard `Mt19937` but with a `ulong` wordtype should
not produce different results to the standard `uint`-based generator.
This patch adds unittests for the first and 10_000'th values of the
sequences generated by a variety of Mersenne Twister implementations
with non-standard template parameter values.
The values in this unittest can be validated by comparison to C++11 by
compiling and running the following C++ program:
/****************************************************************/
template<class UIntType, size_t w>
void mt_result ()
{
std::mersenne_twister_engine<
UIntType, w, 624, 397, 31,
0x9908b0df, 11, 0xffffffff, 7,
0x9d2c5680, 15,
0xefc60000, 18, 1812433253> gen;
gen.seed(std::mt19937::default_seed);
std::cout << gen() << std::endl;
for (int i = 0; i < 9998; ++i)
gen();
std::cout << gen() << std::endl;
std::cout << std::endl;
}
int main ()
{
mt_result<uint32_t, 32>();
mt_result<uint64_t, 32>();
mt_result<uint64_t, 48>();
mt_result<uint64_t, 64>();
}
/****************************************************************/
Note that the `for` loop in this example advances the generator 9998
times compared to the D unittest's `popFrontN(9999)` because the first
`gen()` call already advances the generator once.
With the required tempering parameter `d` introduced by the previous
patch, we can now introduce the standard 64-bit implementation of the
Mersenne Twister. See https://en.wikipedia.org/wiki/Mersenne_Twister
for an explanation of the chosen constants.
Some minimal unittests have been added similar to those already present
for the 32-bit `Mt19937`. These can be verified by comparison to C++11
by compiling and running the following C++ program:
/****************************************************************/
int main ()
{
static_assert(std::mt19937_64::default_seed == 5489,
"Default seed does not match Phobos!");
std::mt19937_64 gen(std::mt19937_64::default_seed);
std::cout << gen() << std::endl;
for (int i = 0; i < 9998; ++i) {
gen();
}
std::cout << gen() << std::endl;
}
/****************************************************************/
Note that the `for` loop in this example advances the generator 9998
times compared to the D unittest's `popFrontN(9999)` because the first
`gen()` call already advances the generator once.
Fixes Issue #10900 <https://issues.dlang.org/show_bug.cgi?id=10900>.
This patch updates MersenneTwisterEngine's implementation to use the new
and much faster implementation Ilya prepared for `mir.random`.
The original Mersenne Twister algorithm (as per the reference C code by
Matsumoto and Nishimura) first regenerates the contents of the entire
state array, before looping over the resulting values and tempering them
in order to generate the actual variates. Once the entire state array
has been iterated over, the array's values are then regenerated, and so
on and so forth.
Ilya's implementation reworks this idea to intertwine update of a single
entry in the state array with the tempering of another value to produce
the next variate. This ensures that all the registers concerned stay
'hot' in the CPU's memory and hence ensures significant speedup.
(Just as a mention: as an experiment while adapting this code for phobos
I tried splitting apart the lines responsible for updating the internal
state array from those responsible for tempering the next variate, so
that they could be run sequentially instead of mixed up together; this
resulted in a fairly significant speed hit.)
In contrast to most (all?) other Mersenne Twister implementations, this
one iterates backwards through the state array, which allows for a few
extra compiler optimizations that speed up the algorithm. It has thus
been necessary to rework the range-based `seed` method to take account
of this.
Besides the algorithmic change, this patch introduces two new template
variables: an extra tempering variable `d`, and the initialization
multiplier `f`, which brings the template implementation in line with
that of the C++11 standard. These extra template variables are needed
in order to effectively implement the standard 64-bit version of the
Mersenne Twister, which will be added in a follow-up patch.
Finally, this implementation introduces corrections to the handling of
the word size `w` to ensure that the right sequences are produced when
the word size is less than the number of bits in `UIntType`. Unittests
will be added for this in a follow-up patch.
------------------------------------------------------------------------
Where this implementation differs from that in `mir.random`:
Ilya's original design has been reworked as a drop-in replacement for
the existing Phobos implementation, which is currently implemented as a
forward range (although it should ideally be an input range, but that
design flaw should be fixed as a separate issue).
There appears to be no significant speed hit from reworking Ilya's code
as a range rather than as a functor. However, some other aspects of the
original design have required rather intrusive changes in order to get
the full speed benefits without introducing breaking change.
The original MersenneTwisterEngine allows for implicit instantiation of
generator instances without providing a seed. This is handled by
checking in the `front` and `popFront` method whether the generator has
been properly initialized, and seeding it with the default seed if not.
However, these runtime checks on every single call result in a massive
speed hit. The current implementation therefore takes a number of steps
in order to ensure that the internal state of the generator can be set
to its default-seeded values at compile time:
* all the internal state variables have been wrapped up in a nested
`State` struct to facilitate generation;
* the internals of the `seed` and `popFront` methods have been
separated out into CTFE'able private static methods (`seedImpl`
and `popFrontImpl`) which take a reference to a `State` instance
as input;
* a CTFE'able private static `defaultState` method has been added
which generates a `State` instance matching that generated by
instantiating the generator with the `defaultSeed`.
The `defaultState` method is then used to initialize the default value
of the internal `State` instance at compile-time, replicating the effect
of the original runtime seeding check.
These latter workarounds could be removed in future if the generator
were updated to `@disable this();` and therefore always require explicit
seeding, but this would be a breaking change and so is avoided for now.
This is added purely in order to verify that generator behaviour will
not change with upcoming code changes. It does not appear to match the
behaviour of the superficially similar sequence-based seeding available
for the C++11 <random> implementation of the Mersenne Twister.
