{simulist} full package review #33
Conversation
…and age-dependent rates
|
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
There was a problem hiding this comment.
Thanks for putting together, lots of useful functionality here. I've mostly focused on the vignettes – have suggested some tweaks to help users understand value of the package, as well as ensuring consistency in terminology etc. throughout.
The dependency on bpmodels is probably worth updating once stable epichains available. Would also be nice to include simulist outbreaks as comparisons in other packages (e.g. to show performance of CFR, EpiNow2, or superspreading methods)
There was a problem hiding this comment.
Thanks, this looks in a good shape for a release very early next year. I like the current interface.
I have left my comments inline but here is a summary of the main ones:
- I'm still unsure about passing
epidistobjects directly rather than functions - I'm proposing a different internal package structure for the next version. This would be an internal refactoring so it doesn't need to delay the first release
There was a problem hiding this comment.
I've tested the functions as they appear on the vignettes and everything works well, I've left some comments, mostly related to changes that could be made to facilitate the understanding of the contents by a first time reader, but overall it looks great and it's a well rounded package, I like how you've solved the issue of simulating both the cases and the contacts for the same outbreak in a way that is so intuitive for users
| head(contacts) | ||
| ``` | ||
|
|
||
| If both the line list and contacts table are required, they can be jointly simulated using the `sim_outbreak()` function. This uses the same inputs as `sim_linelist()` and `sim_contacts()` to produce a line list and contacts table of the same outbreak (the arguments also have the same default settings as the other functions). |
There was a problem hiding this comment.
Maybe also worth adding when/why a user might want to simulate only contacts or contacts as part of the same outbreak with sim_outbreak?
There was a problem hiding this comment.
Maybe also worth adding when/why a user might want to simulate only contacts or contacts as part of the same outbreak with sim_outbreak?
Thanks for the comment, I like the idea, but I'm unable to think of a use case/case study that would fit, other than something trivial, e.g. teaching, etc. Could you provide an example of when a user might want each type of dataset?
|
Thank you for all the comments and suggestions. There have been lots of improvements to the package from this full package review. I have made all changes in separate PRs which are linked to at the bottom of this PR. I will continue working through the release checklist (#32) and then will release the package. |
This PR is to provide a platform to review the entirety of the package.
Once this review concludes I will release v0.1.0 on GitHub. (#32).
This PR is unconventional as it is not intended for merging or for additional commits (unless minor) and instead comments will be converted to issues and these will be addressed in their own PRs.