C++: Why is this method returning the same random number every time?

Question

I have a class which has some data stored in a vector and a method that is supposed to select a random element and return it, but when I run it, it returns the same element every time.

Here is a simplified example based on my code:

#include <iostream> #include <random> #include <vector> class MyObj{ private: std::vector<int> set_data; public: MyObj(int num_elements){ for (int i = 0; i < num_elements; ++i){ set_data.push_back(i); // just so that there is some data in there } }; int getRandomElement(std::mt19937 rng){ std::uniform_int_distribution<int> uni(0,set_data.size()-1); int idx = uni(rng); return set_data[idx]; }; }; int main() { std::random_device r; std::seed_seq seed{r(), r(), r(), r(), r(), r(), r(), r()}; std::mt19937 rng = std::mt19937(seed); MyObj temp(50); for (int i = 0; i < 20; i++){ std::cout << "getting random element: " << temp.getRandomElement(rng) << std::endl; } }

and the output is:

getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19 getting random element: 19

Is there anything obvious that i have done wrong here?

Youre passing the rng by value which means the state change in the function doesnt get applied to the one in main so every call is "independent" (this is also probably really slow because std::mt19937 has a lot of state AFAIK) — Borgleader
– Borgleader, Commented May 8, 2018 at 14:39
This doesn't address the question, but do you really need the extra stuff that std::endl does? '\n' ends a line. — Pete Becker
– Pete Becker, Commented May 8, 2018 at 15:05
@PeteBecker: you're probably right, but back when I was learning c++ at uni, our lecturer always drilled into us that we should get into the habit of using std::endl` and i guess it stuck.. if i was writing something like std::cout << "hello\n";, that would make more sense than std::cout << "hello" << std::endl;, but as i would have to start a new string anyway, it isn't very far from "\n" to std::endl.. also i find it helpful to flush the buffer when debugging stuff.. — guskenny83
– guskenny83, Commented May 8, 2018 at 15:37

Xirema · Accepted Answer · 2018-05-08 14:39:33Z

5

int getRandomElement(std::mt19937 rng){ std::uniform_int_distribution<int> uni(0,set_data.size()-1); int idx = uni(rng); return set_data[idx]; };

This is passing by value, which means the state of the RNG is getting copied from the original, and the original is never getting updated. Changing this method to pass by reference should fix the issue.

int getRandomElement(std::mt19937 & rng){

answered May 8, 2018 at 14:39

Xirema

19.9k4 gold badges37 silver badges70 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

François Andrieux Over a year ago

The distribution objects are allowed to be stateful. It's a good habit to preserve them when generating numbers that are expected to be part of the same distribution rather than recreating them every time.

guskenny83 Over a year ago

thanks! that worked perfectly! @FrançoisAndrieux: generally I am randomly selecting an element so that it can be removed from the set, so the distribution will change nearly every time I need to generate a number.

Collectives™ on Stack Overflow

C++: Why is this method returning the same random number every time?

1 Answer 1

2 Comments

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Related