pele-python / mcpele

Monte Carlo and parallel tempering routines built on the pele foundation
Other
20 stars 5 forks source link

static random number generator in takestep #15

Closed js850 closed 10 years ago

js850 commented 10 years ago

I'm concerned about the static random number generator in takestep

class RandomCoordsDisplacement: public TakeStep{
    static std::mt19937_64 _generator;
}

that means that all instances of RandomCoordsDisplacement will have exactly the same generator. this can't be a good thing. if a new RandomCoordsDisplacement object is created and seeds _generator, then that will change the output of all the instances

I think this is a bug

js850 commented 10 years ago

same with Metropolis and anywhere else there are static generators

kjs73 commented 10 years ago

The reason why I made the random number generators static was to avoid danger from deleting and re-allocating takestep (or other similar things). I agree that for now this made it worse because the generator is seeded in the constructor. In general I think it is not a problem to have the same generator for all "takestep", Metropolis, etc.

To resolve this, there are probably two:

  1. Make the generators non-static, keep seeding in the constructor, and take care not to delete and re-allocate steps or tests.
  2. Keep the generators static and remove the seeding in the constructors. Then the seed function could be called somewhere in the MC constructor, if needed.

Do you have a preference for one of them?

On 8 July 2014 17:53, Jacob Stevenson notifications@github.com wrote:

same with Metropolis and anywhere else there are static generators

— Reply to this email directly or view it on GitHub https://github.com/pele-python/mcpele/issues/15#issuecomment-48367543.

js850 commented 10 years ago

to avoid danger from deleting and re-allocating takestep

why is this such a problem? As long as it is reseeded appropriately.

I would make the generators non-static and make the seed a mandatory argument for the constructor. You can seed it from the python RNG.