-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow events and variables to be added when restoring the simulation. #194
Conversation
The simulation restoring feature required the number of events to be identical to the original simulation's. This has turned out to be too restrictive for our use cases in malariasimulation, where we want to resume the simulation with new interventions enabled, which come with new events. I had originally hoped that this would be enough, and that we could run the first phase of the simulation with all the events present, even though they are unused. However there are interventions whose number of events depends on the parametrization of it, hence we cannot create a saved simulation state that fits all possible use cases when resuming. The solution implemented here is to allow more events and variables to be introduced when resuming the simulation, on the condition that the objects are named when passed to `simulation_loop`. Without this requirement, given more objects than were saved, it would be ambigous which ones need to be restored and which ones are new. When a new object is included in the simulation, its `restore_state` method is called with a NULL argument. This is needed because events, even new ones, still need to set their internal timestep variable. Additionally, the list of events and variables can now be structured using nested lists of objects. This has no impact on the way the simulation is executed, but it allows for more complicated simulations to be restored. For example, this is a simplified example of what the list of events in malariasimulation might look like: ``` list( mda_administer = Event$new(), mass_pev_doses = list( TargetedEvent$new(n), TargetedEvent$new(n), TargetedEvent$new(n) ) ) ``` In this example, the top-level list is names, allowing the `mass_pev_doses` events to be absent during the warmup simulation and added only later when restoring. However, because the nested list of targeted events is not named, more events cannot be added to it. The names of the methods to save and restore state, together with the helper functions `save_object_state` and `restore_object_state` are tweaked and made public to allow this pattern to be reused in applications that have their own state to save, as demonstrated by the [stateful.R] file in malariasimulation. [stateful.R]: https://github.com/mrc-ide/malariasimulation/blob/b3376d33cda29cc5e4d7217fefad4b367f9f9167/R/stateful.R
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #194 +/- ##
==========================================
+ Coverage 96.13% 96.26% +0.12%
==========================================
Files 36 36
Lines 1839 1872 +33
==========================================
+ Hits 1768 1802 +34
+ Misses 71 70 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there's some renaming and minor refactoring too. Which looks fine.
R/simulation.R
Outdated
keys <- NULL | ||
reset <- seq_along(objects) | ||
} else if (is_uniquely_named(objects) && is_uniquely_named(state)) { | ||
missing <- setdiff(names(state), names(objects)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we issue a warning? We could run a simulation with the given state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah not really sure what to do about it. I agree it would totally be possible to run the simulation like that.
I don't think a warning is that useful, we should either support or not at all. Right now I don't really envision a use case where you would really need to remove objects - worst case you can always add them but not use them, or for example for an Event re-add previously scheduled timesteps that are all in the past, meaning the event never fires.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to implement something to support it? Or not raise the error?
The worst case seems like a common case for malariasimulation. I imagine people will want to distribute historic runs with all the interventions turned on for general use. Then researchers interested in a specific intervention may want to focus on that.
You think it's easier to add those empty events (and maybe variables I'm not sure) in malariasimulation so that people don't get errors when users continue a run?
It seemed easier to me for individual to ignore that state when restoring (probably even without a warning!), but I may be missing something here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed and removed that check. It now works even when adding and removing objects.
To be honest my plan with malariasimulation is to do the opposite, have a minimal historic run (that may include the interventions that affect the past, but no more), and individual researchers can add more interventions. I guess we'll see if removing those interventions ever turns out to be useful.
Oh, can you change the destination to dev before merging? Didn't notice that before I approved. Thanks! |
The simulation restoring feature required the number of events to be identical to the original simulation's. This has turned out to be too restrictive for our use cases in malariasimulation, where we want to resume the simulation with new interventions enabled, which come with new events.
I had originally hoped that this would be enough, and that we could run the first phase of the simulation with all the events present, even though they are unused. However there are interventions whose number of events depends on the parametrization of it, hence we cannot create a saved simulation state that fits all possible use cases when resuming.
The solution implemented here is to allow more events and variables to be introduced when resuming the simulation, on the condition that the objects are named when passed to
simulation_loop
. Without this requirement, given more objects than were saved, it would be ambigous which ones need to be restored and which ones are new.Additionally, the list of events and variables can now be structured using nested lists of objects. This has no impact on the way the simulation is executed, but it allows for more complicated simulations to be restored.
For example, this is a simplified example of what the list of events in malariasimulation might look like:
In this example, the top-level list is names, allowing the
mass_pev_doses
events to be absent during the warmup simulation and added only later when restoring. However, because the nested list of targeted events is not named, more events cannot be added to it.The names of the methods to save and restore state, together with the helper functions
save_object_state
andrestore_object_state
are tweaked and made public to allow this pattern to be reused in applications that have their own state to save, as demonstrated by the stateful.R file in malariasimulation.