make MersenneTwister() randomly seeded

[rfourquet]

I have been meaning to propose this change for a while: the default constructor becomes MersenneTwister() = srand(MersenneTwister(0)) instead of MersenneTwister() = MersenneTwister(0). In other words, rand(MersenneTwister()) will yield changing values. I couldn't find a discussion on this topic, and it was like that since the "beginning" (68049d4).

I guess this comes from Matlab behavior; an example of a language which made the other choice is python, where an object random.Random() is seeded randomly by default. In Julia the global RNG is randomly seeded, but explicit instances of MersenneTwister are not which appear a bit odd to me.

In the work I do, I never rarely want the repetitive behavior of MersenneTwister(0), so I always call r = srand(MersenneTwister()) to get a new RNG r. Not a big deal, but IMHO if one needs a specific seed, one should be stating that explicitly with MersenneTwister(0). Furthermore, as this RNG stores the seed it was initialized with, it's pretty easy to re-generate a particular sequence if needed.

Will update the breaking section of NEWS.md if there is support for this change.

[rfourquet]

Bump! Would be happy to hear some opinions on this change.

[andreasnoack]

I think we should make this change.

[StefanKarpinski]

💯 Yes, I would be very badly surprised if I called this and got an implicit seed of zero. I even think this is worth violating the feature freeze since this is definitely not how this should work.

[KristofferC]

I agree with the change but looking through a few of the pages in https://github.com/search?utf8=%E2%9C%93&q=MersenneTwister%28%29+language%3AJulia&type=Code, this will be quite breaking.

[StefanKarpinski]

Most of the calls have a seed argument – how about deprecating the no-argument constructor for now so that we can reintroduce it with random seed later?

[ViralBShah]

I feel that we should just not have a no-argument version at all, and always require an implicit seed. The only exception should be the rand() that uses the GLOBAL_RNG, for convenience.

[rfourquet]

So there is consensus for at least removing the current behavior :)

and always require an implicit seed.

The problem is when one wants a random seed. The current way srand(MersenneTwister()) is OK but it seems so natural to have the default constructor give a randomly seeded RNG... and I don't see why the GLOBAL_RNG would have to be an exception.

Would the way forward be to deprecate MersenneTwister() now, and to introduce the new behaviour (if it is decided upon) in the subsequent release?

[ViralBShah]

I could be ok with having a convenience method for the random seed case too. If we are going to change the behaviour this release, we should do it now for 0.6, so that 1.0 can have the final new behaviour.

[rfourquet]

So I don't know the deprecation policy for this case: if we deprecate now MersenneTwister() for 0.6 and re-enable it (with different behavior) for 1.0, then we don't follow the rule in deprecated.jl "A function deprecated in a release will be removed in the next one." Can we just instead make this call directly an error now (instead of a deprecation)?

[tkelman]

Can we just instead make this call directly an error now (instead of a deprecation)?

No, that would be rather breaking, and we're in feature freeze so technically it's too late to be adding new deprecations. Exceptions could be made, but we better be sure about them.

Not all deprecations have to be deleted later, some could be for behavior changes.

[rfourquet]

I rebased and made the deprecation. I am not competent to decide whether this can be merged for 0.6 (just noting that it would be great to have the new behavior for 1.0).

[tkelman]

It's pretty late to be adding new deprecations for 0.6 since we've already had an alpha and just tagged beta, but since this one is so trivial I'd be okay with it if you're willing to go make PR's that fix it. Looks like it's only used in 9 registered packages: https://gist.github.com/44be8ad4da005a3db07108dc6520d6f2

[rfourquet]

Thanks @tkelman your list of packages to patch was super helpful.

[ViralBShah]

These all seem like reasonably widely used and maintained packages.

[StefanKarpinski]

The only package PR that hasn't been merged is the DSGE one, so we can go ahead with this as soon as CI passes.

[rfourquet]

An old discussion on this topic, which eventually agrees with the change here: #67