Mindwerks / worldengine

World generator using simulation of plates, rain shadow, erosion, etc.
MIT License
982 stars 128 forks source link

The random generator now properly uses the seed to produce deterministic results. #128

Closed tcld closed 8 years ago

tcld commented 8 years ago

I am not sure what the wanted behaviour is, but this looked like it might be unintentional. Since the seed wasn't used to seed the random generator, successive calls to random() of any kind (which occur at several points in the code) gave actual random results, even when a seed was given.

This patch makes the seed be the actual seed of the random generator. This got rid of some of the problems I was having when trying to modify the ocean_level variable so that it can actually modify the ocean level. That is not done yet, however.

It also got rid of the remaining lack of deterministicness I saw when regenerating a world with the same seed (there were some slight changes in colors...hopefully weren't my eyes).

If this proves useful, the seed-variable could probably be removed as a parameter from several function calls. In theory it should not be needed anywhere after having set off the RNG.

tcld commented 8 years ago

If somebody found the time to explain what "coverage" is and how it tends to criticise files that were not changed, I would very much appreciate that. :)

esampson commented 8 years ago

While I somewhat understand 'coverage' I would appreciate if someone could explain what tests are run and how we can add new coverage tests. That way I can check any code I write before submitting it to see if it will fail coverage test and if necessary add a test for my new code.

psi29a commented 8 years ago

Due to the random nature of some of the tests... for example the smoke test, that it sometimes covers code paths in one run in not in others. As we write more tests, this variability will decline.

Coverage basically means "code coverage". When a line of code isn't covered it means that it is never run. This can mean that it is dead-code or that we haven't written tests that cover that specific functionality.

tcld commented 8 years ago

Thanks for the clarification. :)

So more methods are needed in the test-module, I will keep it in mind.

psi29a commented 8 years ago

random.seed() seeds the random number generator with the system's current time. This is actually is redundant code because when using random functions, it will automatically seed with the system's current time.

I recommend you remove the seed.random() from 394.

That you have random.seed(seed) afterwards is great, because it will make the system more deterministic when you define a seed.

tcld commented 8 years ago

In other languages I have used there was always need to initiate the random generation with a single call to seed(). I will double check if this is necessary and adjust the code if necessary.

psi29a commented 8 years ago

https://docs.python.org/2/library/random.html#random.seed

Initialize the basic random number generator. Optional argument x can be any hashable object. If x is
omitted or None, current system time is used; current system time is also used to initialize the 
generator when the module is first imported.

So, if nothing is given... it uses current system time.

When the random module is imported, it will auto initialize with random.seed().

So running it again is redundant.

tcld commented 8 years ago

No, it means if you run random.seed() it uses random.seed(systemtime()). I am still reading, give me a couple of minutes.^^

EDIT: On the same page it says this: seed(): "Initialize the basic random number generator." That doesn't sound optional.

tcld commented 8 years ago

Just tested it in the console (Python 3), you seem to be correct. Although I am fairly certain that somebody has to call seed(), so removing the line will probably not give an advantage in speed.

psi29a commented 8 years ago

Well, not exactly redundant, it gets a new seed... system time. But for our use, is totally redundant as you only need a random number when you didn't give a 'seed'. Now that you have this new seed, you seed the random.seed(seed) which will be used throughout which gives us predictable determinism (thankfully). You also pass this seed on to platec.

So as I see, drop the first random.seed() in your PR for being not necessary. The original (at the top) was also redundant.

psi29a commented 8 years ago

Please re-read what I quoted:

"current system time is also used to initialize the generator when the module is first imported."

That is why you don't need to run random.seed() by itself, unless you want to use another time as a seed.

tcld commented 8 years ago

Sorry, I missed that last part. Thank you for the clarification. :) Commit is up.

tcld commented 8 years ago

As a bonus: every commit gives a new shot at the coverage-test due to the involved randomness. Maybe the test works out this time.^^

EDIT: Well, it's a little bit better.

psi29a commented 8 years ago

;) It's great

Thanks!