-
Notifications
You must be signed in to change notification settings - Fork 5
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
Recording the serial interval #267
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #267 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 110 110
Lines 3876 3978 +102
==========================================
+ Hits 3876 3978 +102 ☔ View full report in Codecov by Sentry. |
@mghosh00 just following up on this so the repo is up to date for publication - how much of this is actually intended to go onto main? For example I see the (untested) triple sweep work here, which I assume was just a simple debugging test and so will not actually be merged into the main repo. If this is the case, it could be more appropriate to close the PR, and perhaps delete the branch if we do not now need this code. |
Closes #273 |
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.
Overall great! Thank you for excellent testing of this code too, as I know we wanted to get this feature added quickly. A few additional files you might want to get rid off, and one code change, but on this basis I am happy to pre-approve.
For future code, I'd perhaps reconsider your commenting style - in general we try to aim for self-documenting code, where the variables names are sufficiently self explanatory that you don't need an additional comment to say what each line does because the code explains itself. Comments are then used sparingly to document unusual structures or use cases. I am happy to discuss this in more detail, but you don't need to make those modifications to this (unless you want to).
For an example, take L490 of pyEpiabm/pyEpiabm/routine/simulation.py
:
# The below is a list of dictionaries for each time step
list_of_dicts = df.to_dict(orient='records')
for dict_row in list_of_dicts:
# Write each time step in dictionary form
self.serial_interval_writer.write(dict_row)
The variable names in this are already pretty meaningful, so the comments add very little additional detail that is not in the code. You could take the comments out (and perhaps just change one of the variable names to make it clear that each row corresponds to a timestep).
Summary
I have added the ability to record serial intervals of all infection events (which can be used to generate a distribution of serial intervals). The output of this is in a file named serial_intervals.csv.
Checklist
All new functions have docstrings in the correct style
I've verified the complete docs build locally without errors
I've maintained 100% coverage (please mention any 'no cover' annotations explicitly)
I've unit-tested all new methods directly
Breaking change (fix or feature that would cause existing functionality to change)
I have read the CONTRIBUTING document.
My code follows the code style of this project.
I have updated the wiki with new parameters/model functionality
Closing issues
Closes #262