-
Notifications
You must be signed in to change notification settings - Fork 16
Include option to use emulator in RTI module #1756
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
base: master
Are you sure you want to change the base?
Conversation
…rience the incident at the time of the polling, ensure that the end of the episode is calculated from the stored incident time in case this is updated in the future.
…ess outputs and generate data
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.
Thanks, Marghertia. We could tidy this up by having a new EmulatedRTI class that is separate from the RTI class. If there are things that need to be shared, you could make a base class (e.g. BaseRTI) and have both RTI and EmulatedRTI inherit that. But you may not need to because most of the code is not run in the EmulatedRTI.
Each on these classes could have their own polling event class or, if they too share code (looks like it does) the common bits could be extracted into a shared helper function or, similarly, make a BaseRTIPollingEvent and have the simulated and emulated event class use/inherit.
This would avoid the long if-else blocks and keeps the emulated RTI stuff together and separate. You might not need the 'use_emulated' in the end because a different module will be registered at simulation start.
| @@ -1,3 +1,4 @@ | |||
| 0 | |||
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.
Remove
| 'age_years', 'sex', 'prob_of_dying_before_next_poll']].copy() | ||
|
|
||
|
|
||
| # Artificially increase risk for men under 50 by 50 |
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.
This should probably be removed for the merge...?
| self._write_hsi_event_counts_to_log_and_reset() | ||
| self._write_never_ran_hsi_event_counts_to_log_and_reset() | ||
|
|
||
|
|
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.
Remove extraneous newline
| def write_to_log_and_reset_counters(self): | ||
| """Log summary statistics reset the data structures. This usually occurs at the end of the year.""" | ||
|
|
||
|
|
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.
Remove newline
| df.loc[DALYweightunderlimit, 'rt_disability'] = 0 | ||
| assert (df.loc[injured_index, 'rt_disability'] > 0).all() | ||
|
|
||
|
|
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.
Remove newline
|
|
||
| def run_simulation_to(self, *, to_date: Date) -> None: | ||
| """Run simulation up to a specified date. | ||
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.
Rollback both changes in this file
| Types.INT, | ||
| "limit on the number of times an HSI event can run" | ||
| ), | ||
| 'use_RTI_emulator': Parameter( |
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.
Rename this to use_emulator - you're already in RTI
|
Also need to add some tests for emulated RTI. |
This PR offers users the option to use an emulated version of the RTI module. It will require #1758 to run.
@tamuri, a couple of "conceptual" issues:
True? (Not very elegant)Many thanks,
Margherita