Parametrised database reinitialisation #144
Conversation
james-whiteside
requested review from
alexjpwalker and
haikalpribadi
as code owners
| val randomSource2 = randomSource.nextSource() | ||
| if (context.recreateDatabase) init(randomSource2) |
There was a problem hiding this comment.
So when recreateDatabase is false, we generate a new random source but just discard it?
While this is probably harmless, it reads awkwardly. We should not do that.
| val randomSource2 = randomSource.nextSource() | |
| if (context.recreateDatabase) init(randomSource2) | |
| if (context.recreateDatabase) init(randomSource.nextSource()) |
There was a problem hiding this comment.
Just noticed the comment in the PR description about incrementing the master random source whether the DB is recreated or not.
If that's the behaviour we want, then fine - I'd say it has the potential to blindside the reader though! Could we add a comment in the code itself clarifying that yes, this is really what we want to do here (and why)?
There was a problem hiding this comment.
Yes, that is the best way to ensure the random selections remain deterministic. Resolved as discussed.
There was a problem hiding this comment.
The following change was tested (lines 39 and 40):
if (context.recreateDatabase) init(randomSource.nextSource())
else randomSource.nextSource()
however this did not produce deterministic results. The following unit test was run:
@Test
fun testSource() {
val seed: Long = 0
val randomSourceA = RandomSource(seed)
val randomSourceB = RandomSource(seed)
assert(randomSourceA.nextInt(1000000) == randomSourceB.nextInt(1000000))
val randomSourceA2 = randomSourceA.nextSource()
repeat(1000000) {
randomSourceA2.nextBoolean()
}
if (false) {}
else randomSourceB.nextSource()
assert(randomSourceA.nextInt(1000000) == randomSourceB.nextInt(1000000))
}
which passed. It is unknown why these changes to lines 39 and 40 affect the determinism of the simulation. An issue has been opened here.
What kind of use cases? Could we add an example to the PR description? |
Have edited the description a bit to be more descriptive. |
What is the goal of this PR?
Running multiple simulations on the same database without reinitialising it allows simulations to be run incrementally, which is very useful for exploring the way the data changes throughout the simulation for both research and bug fixing. This PR makes that possible.
What are the changes implemented in this PR?
The Config class has been modified to include a new
recreateDatabaseparameter. The Context and Simulation classes have been updated accordingly. In order to ensure that repeated runs of the same simulation are deterministic, the Agent class has been modified so that each spawned agent now receives its ownRandomSource. The Simulation class has also recieved a small modification to allow this, by ensuring that the master random source is incremented regardless of whether the database is recreated or not. Finally, the Simulation class has also been modified to produce a slightly clearer output by including the number of times each agent action is run.