Improving my code which is meant to implement a function that picks a random number uniformly between 0 and 1

[Paul]

The following code (which actually works quite well)
is designed to satisfy the brief: "Write a function to
randomly (and uniformly) pick a number between 0 and 1"

My attempt is below.
It received a complaint that the seed shouldn't be reset
each time. However, if you don't do that, then
you just get exactly the same result each time nextDouble()
is called.

Can anyone make any sense of this complaint?
I'm sorry that the complaint wasn't more detailed than
stated above -- something like "you shouldn't need to reset
the seed each time."
Free use of library functions was allowed with python coders
encouraged to just use random.random() and java coders encouraged
to just use nextFloat()

// Uniform distribution between 0 and 1.
// Seed is reset by incrementing the seed randomly each time the
// function is called.
double RandomGen::nextDouble()
{
static long long seedCounter = 0;
std::default_random_engine generator;
generator.seed(seedCounter);

seedCounter += rand();
/* The simpler ++seedCounter does not work.
It leads to relative errors that are too small to be credible. */

std::uniform_real_distribution<double> distribution(0.0, 1.0);
return (distribution(generator));
}

[Alf P. Steinbach]

On 06.10.2018 18:45, Paul wrote:
> The following code (which actually works quite well)
> is designed to satisfy the brief: "Write a function to
> randomly (and uniformly) pick a number between 0 and 1"
>
> My attempt is below.
> It received a complaint that the seed shouldn't be reset
> each time. However, if you don't do that, then
> you just get exactly the same result each time nextDouble()
> is called.

No. But with old version of g++ you get the same /sequence/ every time.

Oh look, I answered that on Stack Overflow 3 days ago, what a
coincidence! <url:
https://stackoverflow.com/questions/52625317/c14-2-random-generators-one-works-the-other-doesnt/52625445#52625445>

Note: I'm at reputation 1 there again, because I used the "i" word
(incompetence) again. It was about someone first voting to close a very
reasonable and clear and interesting question, then not admitting it was
wrong to do that and instead asserting that there could be no answer
because it was too broad, which it was not, then sort of instantly
downvoting my answer delineating the exactly two ways to do it at the
core language level. I didn't name him, just wrote that whoever
downvoted my answer displayed an extraordinary incompetence. I think
they've automated the suspension process for me, and for other misfits
who use clear Descriptive Words™. :)

> Can anyone make any sense of this complaint?
> I'm sorry that the complaint wasn't more detailed than
> stated above -- something like "you shouldn't need to reset
> the seed each time."
> Free use of library functions was allowed with python coders
> encouraged to just use random.random() and java coders encouraged
> to just use nextFloat()
>
> // Uniform distribution between 0 and 1.
> // Seed is reset by incrementing the seed randomly each time the
> // function is called.
> double RandomGen::nextDouble()
> {
> static long long seedCounter = 0;
> std::default_random_engine generator;
> generator.seed(seedCounter);
>
> seedCounter += rand();
> /* The simpler ++seedCounter does not work.
> It leads to relative errors that are too small to be credible. */
>
> std::uniform_real_distribution<double> distribution(0.0, 1.0);
> return (distribution(generator));
> }

Cheers & hth.,

- Alf

[Alf P. Steinbach]

On 06.10.2018 18:45, Paul wrote:

> [snip]

>
> // Uniform distribution between 0 and 1.
> // Seed is reset by incrementing the seed randomly each time the
> // function is called.
> double RandomGen::nextDouble()
> {
> static long long seedCounter = 0;
> std::default_random_engine generator;
> generator.seed(seedCounter);
>
> seedCounter += rand();
> /* The simpler ++seedCounter does not work.
> It leads to relative errors that are too small to be credible. */
>
> std::uniform_real_distribution<double> distribution(0.0, 1.0);
> return (distribution(generator));
> }
>

Oh sorry, in addition to earlier remarks, you need to /retain state/
from one call to the next.

One is to make the generator `static`, but that would be a kludge.
Instead I'd put the state, i.e. the `generator`, as a member variable in
the `RandomGen` class. The caller has to remember to keep an instance of
that class and not create a new one every time.

Also sorry for inadvertently sending this as a reply mail instead of to
the group. Hm.


Cheers & again hth.,

- Alf

[Tim Rentsch]

"Alf P. Steinbach" <alf.p.stein...@gmail.com> writes:

> On 06.10.2018 18:45, Paul wrote:
>
>> The following code (which actually works quite well)
>> is designed to satisfy the brief: "Write a function to
>> randomly (and uniformly) pick a number between 0 and 1"
>>
>> My attempt is below.
>> It received a complaint that the seed shouldn't be reset
>> each time. However, if you don't do that, then
>> you just get exactly the same result each time nextDouble()
>> is called.
>
> No. But with old version of g++ you get the same /sequence/
> every time.

The code he posted uses rand(). Different sequences can be
obtained by calling srand(), eg, at program startup.

[Tim Rentsch]

"Alf P. Steinbach" <alf.p.stein...@gmail.com> writes:

> On 06.10.2018 18:45, Paul wrote:
>
>> [snip]
>>
>> // Uniform distribution between 0 and 1.
>> // Seed is reset by incrementing the seed randomly each time the
>> // function is called.
>> double RandomGen::nextDouble()
>> {
>> static long long seedCounter = 0;
>> std::default_random_engine generator;
>> generator.seed(seedCounter);
>>
>> seedCounter += rand();
>> /* The simpler ++seedCounter does not work.
>> It leads to relative errors that are too small to be credible. */
>>
>> std::uniform_real_distribution<double> distribution(0.0, 1.0);
>> return (distribution(generator));
>> }
>
> Oh sorry, in addition to earlier remarks, you need to /retain
> state/ from one call to the next.

The code does have retained state, in the static variable
seedCounter. There is also the retained state of rand(),
whatever that may be. This approach may not be a very
good way of doing things, but it does have retained state.

[Alf P. Steinbach]

The code he used, that you chose to snip out, uses
`std::default_random_engine`.

And yes, /also/ `rand`, evidently in an attempt to fix things.

`srand` is irrelevant for fixing this code.

[Alf P. Steinbach]

That's a technically correct observation, but entirely misses the point.

When the code is fixed, the features that you chose to regard as most
important, will be entirely gone.

They're irrelevant, attempted fixes that should just be cleaned out.

[Tim Rentsch]

Paul <peps...@gmail.com> writes:

[edited and rearranged]

> The following code is designed to randomly (and uniformly) pick a
> number between 0 and 1.

>
> // Uniform distribution between 0 and 1.
> // Seed is reset by incrementing the seed randomly each time the
> // function is called.
> double RandomGen::nextDouble()
> {
> static long long seedCounter = 0;
> std::default_random_engine generator;
> generator.seed(seedCounter);
>
> seedCounter += rand();
> /* The simpler ++seedCounter does not work.
> It leads to relative errors that are too small to be credible. */
>
> std::uniform_real_distribution<double> distribution(0.0, 1.0);
> return (distribution(generator));
> }
>

> It received a complaint that the seed shouldn't be reset each
> time. However, if you don't do that, then you just get exactly
> the same result each time nextDouble() is called.
>

> Can anyone make any sense of this complaint? [...]

Generally it's not a good idea to use setting a seed value to get
another random number. The usual advice is seed once, then
extract as many random numbers as you need.

I realize that what you're doing here follows a different
pattern. However it's still not a good idea. The size of the
state space may be greatly reduced. The quality of the
generated seeds may be low (rand() is notoriously unreliable).
And random number generators sometimes take an iteration or
several to "get up to speed" after being seeded.

If it were me I might do something along these lines (please
excuse changes in naming):

class RandomGenerator {
static std::default_random_engine uniform_double_generator;

public:
static double uniform_double();

static void seed_uniform_double( unsigned long long s ){
uniform_double_generator.seed( s );
}
};

std::default_random_engine RandomGenerator::uniform_double_generator;

double
RandomGenerator::uniform_double(){

std::uniform_real_distribution<double> distribution(0.0, 1.0);

return distribution( uniform_double_generator );
}

I added an interface to seed the underlying generator, and
avoided any call to rand(). Usually it's a good idea not
to couple different generators, so numbers used for one
purpose don't interfere with another. The key point though
is to use the underlying generator, and not rely on seeding
to give the randomness.

[Juha Nieminen]

Paul <peps...@gmail.com> wrote:
> It received a complaint that the seed shouldn't be reset
> each time. However, if you don't do that, then
> you just get exactly the same result each time nextDouble()
> is called.

Whoever offered that criticism might have expressed himself abysmally
poorly.

What you are doing in the function you provided is not how RNGs
should be used, nor how they are intended to be used. (What you
are doing there is most probably only going to reduce the quality
of the randomness, possibly making the period of the RNG stream
smaller, possibly a *lot* smaller, and in many cases making it
less efficient.)

The random number generators in the standard library store their
internal state within the object you instantiate, and it's this
object that you should be keeping somewhere, if you want a stream
of random numbers. If the task is "write a function that returns
a value between 0.0 and 1.0 at random", one possible way to do
that is like:

//-------------------------------------------
namespace
{
std::default_random_engine myRNG(0);
std::uniform_real_distribution<double> myDistribution(0.0, 1.0);
}

double RandomGen::nextDouble()
{
return myDistribution(myRNG);
}
//-------------------------------------------

(In many cases this is not the most optimal solution design-wise,
or even functionally (eg. it's not thread-safe), but you should get
the idea.)

[Tim Rentsch]

My comment was on point for the statement it was responding to,
which is quite intentionally the only point it was addressing.
Get the intellectual chip off your shoulder. Your sense of
self-importance is rather larger than is warranted.

[Alf P. Steinbach]

How you deduce "sense of self-importance" from a technical observation
is beyond me, I'm sorry.

It seems to be a psychological mechanism at play in your mind.

Maybe sleep some, go for a walk, talk with people. Listen to music. Read
cartoons, whatever.