search

gap-packages / wedderga

Wedderburn Decomposition of Group Algebras
https://gap-packages.github.io/wedderga/
GNU General Public License v2.0
3 stars 6 forks source link

Add Reset(GlobalMersenneTwister) in some places #15

Closed angeldelriomateos closed 6 years ago

angeldelriomateos commented 6 years ago

This is needed to avoid different output when Random is used to find a generator of a cyclic group

fingolfin commented 6 years ago

Looking at this from the outside, I must say it sounds like a bad idea to reset the random number generator in your code. I'd be really reluctant to allow such a change into the GAP library, or any package I am involved with...

If this is about stability tests, then just make sure you use START_TEST in them, which resets the RNG seed.

If this is for something else, it would be good to know the actual problem to be able to come up with a better solution.

fingolfin commented 6 years ago

Indeed, I notice your bugfix.tst does not use START_TEST, and the last commit to it was about improving stability of a test. You'd not have the problem if you used START_TEST. On the other hand, it can of course be desirable to have a "truly" random tests, to increase the likelihood of uncovering bugs over time. Then again, even if you don't reset the RNG, it does not vary that much during automated test runs (i.e. only if some code that runs before your test now suddenly calls Random a different number of times). So I am not sure this outweighs the complications caused by not having a stable test -- that's of course your choice to make, though :-).

olexandr-konovalov commented 6 years ago

Oops, pressed "merge" on a wrong pull request and merged this accidentally. We will submit a new PR which will address the suggestions properly.