Generically encapsulating random generators and distributions into a class template

Question

I previously had a set of classes in one of my older libraries for working with the random number generators and distributions that was written when Visual Studio 2008 - 2010 were commonplace and before the release of 2012. I was starting a new project in Visual Studio 2017 so I decided to port my original version of the class over. It was giving me a plethora of errors, so I had to modify the original class. When I tried to make a generic function to use the above classes, I ended up facing some downfalls due to the fact that you can not partial specialize function templates. After enough frustration for the past few days, I started over and rewrote the whole class from scratch.

With some of the newer features of C++11 and higher, I was able to use variadic templates which truly simplified things once I got used to the required syntax.

I have this class Generator that is working without errors to the best of my knowledge; it compiles, builds and runs without errors and I have tested a handful of different combinations of engines or generators, different seeding techniques with different distributions.

#ifndef GENERATOR_H
#define GENERATOR_H

#include <limits>
#include <chrono>
#include <random>
#include <type_traits>

enum SeedType { USE_CHRONO_CLOCK, USE_RANDOM_DEVICE, USE_SEED_VALUE, USE_SEED_SEQ };

template<class Engine, class Type, template<typename> class Distribution>
class Generator {
public:
    using Clock = std::conditional_t<std::chrono::high_resolution_clock::is_steady,
        std::chrono::high_resolution_clock,
        std::chrono::steady_clock>;

private:
    Engine _engine;
    Distribution<Type> _distribution;
    Type _value;

public:    
    template<class... Params>
    explicit Generator( Engine engine, Params... params ) : _engine( engine ) {
        _distribution = Distribution<Type>( params... );
    }

    void seed( SeedType type = USE_RANDOM_DEVICE, std::size_t seedValue = 0, std::initializer_list<std::size_t> list = {} ) {
        switch( type ) {
            case USE_CHRONO_CLOCK:  { _engine.seed( getTimeNow() );  break; }
            case USE_RANDOM_DEVICE: { std::random_device device{};
                                      _engine.seed( device() );      break; }
            case USE_SEED_VALUE:    { _engine.seed( seedValue );     break; }
            case USE_SEED_SEQ:      { std::seed_seq seq( list );
                                      _engine.seed( seq );           break; }
        }
    }

    void generate() { _value = _distribution( _engine ); }

    Type getGeneratedValue() const { return _value; }

    Distribution<Type> getDistribution() const { return _distribution; }

    std::size_t getTimeNow() {
        std::size_t now = static_cast<std::size_t>(Clock::now().time_since_epoch().count());
        return now;
    }    
};

#endif // !GENERATOR_H

Using it is as simple as this demonstrating a few examples:

#include <iostream>
#include <iomanip>
#include <vector>
#include "generator.h"

int main() {            
    // Engine, Seeding Type, & Distribution Combo 1
    std::mt19937 engine1;
    Generator<std::mt19937, short, std::uniform_int_distribution> g1( engine1, 1, 100 );
    g1.seed( USE_RANDOM_DEVICE );

    std::vector<short> vals1;
    for( unsigned int i = 0; i < 200; i++ ) {
        g1.generate();
        auto v = g1.getGeneratedValue();
        vals1.push_back( v );
    }

    int i = 0;
    for( auto& v : vals1 ) {

        if( (i % 10) != 0 ) {
            std::cout << std::setw( 3 ) << v << " ";
        } else {
            std::cout << '\n' << std::setw( 3 ) << v << " ";
        }       
        i++;
    }
    std::cout << "\n\n";

    // Engine, Seeding Type, & Distribution Combo 2
    std::ranlux48 engine2;
    std::initializer_list<std::size_t> list2{ 3, 7, 13, 17, 27, 31, 43 };   
    Generator<std::ranlux48, unsigned, std::binomial_distribution> g2( engine2, 50, 0.75 );
    g2.seed( USE_SEED_SEQ, std::size_t(7), list2 );

    std::vector<unsigned> vals2;
    for( int i = 0; i < 200; i++ ) {
        g2.generate();
        auto v = g2.getGeneratedValue();
        vals2.push_back( v );
    }

    i = 0;
    for( auto& v : vals2 ) {    
        if( (i % 10) != 0 ) {
            std::cout << std::setw( 3 ) << v << " ";
        } else {
            std::cout << '\n' << std::setw( 3 ) << v << " ";
        }
        i++;
    }
    std::cout << "\n\n";

    // Engine, Seeding Type, & Distribution Combo 3
    std::minstd_rand engine3;
    Generator<std::minstd_rand, float, std::gamma_distribution> g3( engine3, 0.22222f, 0.7959753f );
    g3.seed( USE_CHRONO_CLOCK );

    std::vector<float> vals3;    
    for( int i = 0; i < 200; i++ ) {
        g3.generate();
        auto v = g3.getGeneratedValue();
        vals3.push_back( v );
    }

    i = 0;
    for( auto& v : vals3 ) {

        if( (i % 5 ) != 0 ) {
            std::cout << std::setw( 12 ) << v << " ";
        } else {
            std::cout << '\n' << std::setw( 12 ) << v << " ";
        }
        i++;
    }
    std::cout << "\n\n";    

    std::cout << "\nPress any key and enter to quit.\n";
    std::cin.get();

    return 0;
}

Is this an appropriate way to generically encapsulate the random generators and distributions? Are there any gotchas that I'm missing? Finally, can this be improved for efficiency purposes?

Answer 1

I like your code. You can upgrade some bits though:

Scoped enums

It is better to use scoped enumerations. They're declared with a slightly different syntax:

enum class Seed { ... }; // enum struct Seed is also valid

They bring additional safety because they can't be implicitly converted to their underlying type (which you can specify, by the way: enum class Seed : char { ... };).

You can then use the enum's values as constants:

switch (seed_type) {
    case Seed::Clock  : ...
    case Seed::Device : ...
    case Seed::Value  : ...
    case Seed::Seq    : ...
}

I'm of the opinion that specifying the scope (in this case Seed::) allows for more concise denominations than those macro-looking upper-case constant names you chose.

Universal references and perfect forwarding

That's a somewhat technical topic you can read about at length in articles and blogs, but just so you know the basics, in the context of template deduction, the double ampersand suffix let the compiler deduce the correct way of passing the argument for you:

// original code: Params are copied. 
// But what if they're big objects, or if they must keep track of a state?
template<class... Params>
explicit Generator( Engine engine, Params... params ) : _engine( engine ) {
    _distribution = Distribution<Type>( params... );
}

// suggested modification:
// the compiler will correctly copy rvalues and pass lvalues by reference
template<class... Params>
explicit Generator( Engine engine, Params&&... params ) : _engine( engine ) {
    _distribution = Distribution<Type>( std::forward<Params>(params)... );
}

The interface is perfectible

I haven't tested your code, but it seems that your seed member function obliges the client to specify a seed value even if they only want to use the sequence parameter. You need to have several seed functions with different overloads, or a templated seed function, or a combination of both.

An example of what you could do:

template <SeedType seed, typename Arg = int>
void seed(Arg&& arg = Arg{}) {
    if constexpr (seed == USE_CHRONO_CLOCK) {
        _engine.seed( getTimeNow() );
    } else if constexpr (seed == USE_RANDOM_DEVICE) {
        std::random_device device{};
        _engine.seed( device() );
    } else if constexpr (seed == USE_SEED_VALUE) {
        _engine.seed( arg);
    } else if constexpr (seed == USE_SEED_SEQ) {
        std::seed_seq seq( arg );
        _engine.seed( seq );
    }
}

And then you would write:

g1.seed<USE_RANDOM_DEVICE>();
// ...
g2.seed<USE_SEED_SEQ>(list2);
// ...
g3.seed<USE_CHRONO_CLOCK>();

Of course, it'd be best to add a few static asserts to check the type of the template argument.

Various

Braces are superfluous around the branches of the switch statement, and they don't really increase readability.

Answer 2

A few observations.

Visibility

Is there any special plan in declaring Clock a public member type? It's not used in any other part of the interface. Currently its possible use is hardly anything more than for a user to maybe tell which clock is used for the chronoinitialization via std::is_same.

In the meantime, Distribution<Type> is left as is, Distribution being a template argument, and it's not even typedefed anywhere inside the class. So, users cannot type something like

MyGenerator::Distribution d = generator.getDistribution();

they should remember, what the Distribution to MyGenerator was originally, or use auto.

Initialization

Splitting object construction and initialization in two phases is generally an antipattern (yes). In your implementation, Generator() and seed() both provide unique ways to initialize an engine. It would make sense to add seeding-related capabilities to the constructor; and to allow for default seed value in seed().

Overloading

seed() in its current form is too complex. A procedure whose only statement is a switch is likely a candidate for splitting into several overloads. Moreover, it does not allow for all the possible ways to initialize an engine.

Return values

Putting seed aside, generate() returning...void? It is ok to store the generated value for a later repeat, but to require that the user do two calls for the price of one? Really Type generate() { return _value = _distribution( _engine ); } seems more helpful. People often want random values; it is much less common that they simply want to order to produce a value, and don't care for what it finally turned out to be.

Too many copies

For instance, engine is passed by-value to the constructor and then copied again into _engine.

Answer 3

There was a similar question recently. The one main gist is that you should not add the random device as a member

C++ Random Number Generation

In the end you also do not really need it once you initialized your Engine. Consequently you should only put the engine in you class.