3

I'm trying to create a std::discrete_distribution object using data passed into a class constructor. I know how to create this using static data, but cannot figure out how using variable data (cleanly). What I have now "works", but is painful. Is there a more appropriate way of doing this?

The distInit = { distArray[0], ... }; line is the problem.

#include <iostream>
#include <iomanip>
#include <initializer_list>
#include <map>
#include <random>

class Die {
    private:
        int loadSide;
        double loadAmount;
        std::mt19937 generator;
        std::discrete_distribution<> distribution;
        std::initializer_list<double> distInit;
        std::array<double, 7> distArray;
    public:
        Die( int loadSide, double loadAmount ) : loadSide(loadSide), loadAmount(loadAmount) {
            distArray.fill( 1 );
            distArray[0] = 0;
            distArray[this->loadSide] = this->loadAmount;

            distInit = { distArray[0], distArray[1], distArray[2], distArray[3], distArray[4], distArray[5], distArray[6] };
            distribution.param( distInit );
        };
        int roll( ) {
                return distribution( generator );
        };
};

const int ROUNDS = 10000;

int main() {
    Die* die = new Die( 5, 20 );

    std::map<int, int> m;
    for(int n=0; n < ROUNDS; n++) {
        m[die->roll()]++;
    }
    for(auto p : m) {
        std::cout << p.first << " generated " << std::setiosflags(std::ios::fixed) << std::setprecision(2) << (float) p.second / ROUNDS << " times\n";
    }
}

I may not be asking the right question, which I will apologize in advance for if so. This is a strong possibility as I'm surprised I'm unable to find any (apparently) related hits on this subject.

My compiler is g++-mp-4.8 (MacPorts gcc48 4.8-20130411_0) 4.8.1 20130411 (prerelease)

Command line /opt/local/bin/g++-mp-4.8 -std=c++11 test.cpp -o test

Improve this question

3 Answers 3

Reset to default
2

If you have variable data, you should be using the discrete_distribution constructor taking a pair of iterators:

template< class InputIt >
discrete_distribution( InputIt first, InputIt last );

You shouldn't be trying to construct the param_type directly; instead use a helper function to construct your distribution:

class Die {
    private:
        std::mt19937 generator;
        std::discrete_distribution<> distribution;
        static std::discrete_distribution<> makeDistribution(
            int loadSide, double loadAmount )
        {
            std::array<double, 7> distArray;
            distArray.fill( 1 );
            distArray[0] = 0;
            distArray[loadSide] = loadAmount;
            return {std::begin(distArray), std::end(distArray)};
        }
    public:
        Die( int loadSide, double loadAmount ) :
            generator{ },
            distribution{ makeDistribution( loadSide, loadAmount ) }
        {}
        int roll( ) {
                return distribution( generator );
        }
};
Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Sigh... I never even looked at the constructor, not that it would have helped. I see the uniform initialization, and will have to make sense of that (it's been way to long since I've used c++). I don't understand makeDistribution returning a std::discrete_distribution type either. More research. Thanks for the help.
2

std::initializer_list only intended for use as a temporary object (function argument) or local variable. It's not a container and it doesn't own anything; it's an accessor to an anonymous, temporary array.

The Standard includes an example similar to your code, §8.5.4/6, which mentions

the initializer_list object is initialized in a constructor’s ctor-initializer, so the array persists only until the constructor exits, and so any use of the elements of i4 after the constructor exits produces undefined behavior.

In your case, it's the body of the constructor, not a ctor-initializer preceding the body, but the story is the same. It's just dumb luck that your program is working for now.

To store the distribution in the object, use std::array or std::vector. array is more efficient but it doesn't support arr = { … } syntax. (There are a few simple alternatives.) vector does support your syntax using braces and the = operator; this support uses an implicit std::initializer_list.

Improve this answer

6 Comments

So, I understand the dump part, but where is the luck of what I have implemented? I'm correctly constructing a initializer_list. Maybe you're thinking I'm going to change the distribution because of the (not needed) class variable distArray? Not the intent, just incorrectly placed.
std::array supports aggregate initialization syntax explicitly as per C++11 23.3.2.1/2 and thus can be used for arr = { ... } type stuff... It just seems GCC 4.7 is reluctant to do it, but GCC 4.8 is fine with it.
@rob It's not correctly constructed. The initializer_list comprises two pointers to an array, and that array is only a local variable. It persists only until the constructor exits, after which time the pointers are dangling.
@Potatoswatter I'm confused by your statement "The initializer_list (distInit) contains two pointers". Can you clarify? I only had class variables in the OP.
@Rob What are you confused by? The initializer_list object does not properly own its contents. It does not perform new or delete. The contents are an array which is local to whatever scope contains the initialization… in this case, the body of the constructor. Do not use initializer_list as a class member.
|
0

I don't know any better way to create a std::initializer_list from a container like std::array other than the one shown in the OP.

However, for the original problem, namely, passing the parameters to the distribution, I can suggest something simpler.

    typedef std::discrete_distribution<>::param_type param_type;
    distribution.param(param_type(distArray.begin(), distArray.end()));

The standard says that distributions must provide a type member param_type (which is the type of argument taken by param()) but doesn't specify it. However, [rand.req.dist] says that

For each of the constructors of D [the distribution type] taking arguments corresponding to parameters of the distribution, P [param_type] shall have a corresponding constructor subject to the same requirements and taking arguments identical in number, type, and default values.

Well, it turns out that std::discrete_distribution<> has a constructor taking iterators pointing to the range of parameters. Therefore, whatever std::discrete_distribution<>::param_type is, it must have a similar constructor. Therefore, I'm suggesting creating a param_type from distArray.begin() and distArray.end() and pass it to distribution.param().

A side note: You no longer need std::initializer_list<double> distInit; in your class. It seems to me that you don't need std::array<double, 7> distArray as a class member either (it could be a local variable in Die's constructor).

Improve this answer

3 Comments

Ahhh. I had tried the iterator at some point and came up empty. I had not cast it to a std::discrete_distribution<>::param_type. Thanks for the help.
@Rob: I'm glad that it helps. Notice that in your particular case, the solution by ecatmur, namely, using the constructor is indeed superior to mine. However, my argument is to be used when you need to change the parameters for an already built distribution.
Yes, 100% agree with your assessment. I didn't state my true requirement correctly, rather what I thought I needed.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.