{superspreading} v0.2.0 pkg review #77
Conversation
…smission vignette
…ganisation contributiing guide
…rit organisation templates
…rganisation template
|
Hi @joshwlambert, when would you like us to return feedback? |
|
1-2 week turnaround time would be good. |
There was a problem hiding this comment.
Thanks!
For a process point of view, I slightly prefer using the first commit as the target branch since it allows commenting on all portions of the code. One big benefit of full package reviews for me is to view the entire code and consider its design as a whole rather than seeing just the more recent changes.
It's still manageable though so curious to see if others found it worked better for them.
| ## New features | ||
|
|
||
| * A new function (`calc_network_R()`) to estimate the reproduction number for heterogeneous networks and a vignette outlining use cases for the function from existing epidemiological literature is added (#71). | ||
| * `proportion_*()` functions can now return proportion columns of the output `<data.frame>` as `numeric` when the new argument `format_prop` is set to `FALSE` (#72). |
There was a problem hiding this comment.
Should this be a new argument of a separate function we can pipe to?
There was a problem hiding this comment.
I see your point and hadn't thought about it as a function you could pipe to. However, I still think this is the simplest implementation and user experience. Some of the thought behind the implementation is in the design vignette, and the argument is after the ... so I don't think users will be accidentally switching the formatting.
|
|
||
| The {superspreading} package aims to provide a range of summary metrics to characterise individual-level variation in disease transmission. These include calculating the probability an outbreak becomes an epidemic (`probability_epidemic()`), or conversely goes extinct (`probability_extinct()`), the probability an outbreak can be contained (`probability_contain()`), the proportion of cases in cluster of a given size (`proportion_cluster_size()`), and the proportion of cases that cause a proportion of transmission (`proportion_transmission()`). | ||
|
|
||
| The other aspect of the package is to provide probability density functions and cumulative distribution functions to compute the likelihood for distribution models to estimate heterogeneity in individual-level disease transmission that are not available in R (i.e. base R). At present we include two models: Poisson-lognormal (`dpoislnorm()` & `ppoislnorm()`) and Poisson-Weibull (`dpoisweibull()` & `ppoisweibull()`) distributions. |
There was a problem hiding this comment.
Is this really a goal or more a means to an end? Relates to epiverse-trace/epiverse-trace.github.io#162.
There was a problem hiding this comment.
I guess it's a bit of both. Mostly to be able to fit these distributions, but users may find other utility for them that I'm not foreseeing.
On https://github.com/orgs/epiverse-trace/discussions/162 I don't have much to add, other than to say I like the idea and have upvoted it on the discussions board. Happy to help if this discussion becomes a project/package.
There was a problem hiding this comment.
{superspreading} v0.2.0 review
Thanks for this work @joshwlambert, it all looks good to me. I left comments on specific lines and refrained from commenting on aspects that were already touched by other reviewers. I will leave other comments below for cases where I couldn't comment on the specific line.
Other comments
-
Resolution of time units in
calc_network_R()is not clear. It seems the current time unit assumes years, but this is not clear from the documentation. It would be good to have a clear statement of the time unit in the documentation. Is it possible to allow the function to be used for other time units (e.g. days, weeks, months)? -
proportion_cluster_size(): The top part of the documentation can be shortened as it is a bit repetitive. In particular, the title can be shortened and the "details" and "description" sections combined to provide more context. -
proportion_transmission(): the title and description could be shortened and the rest moved to the description if they provide more context. -
In multiple functions (
probability_contain(),probability_epidemic(),proportion_cluster_size(),proportion_transmission()), there seems to be a duplication in the input checks and error messages. This makes them a good candidate to combine them into a single function.
Thanks for the suggestion, I tried bundling the input checking into a single function to prevent replicating checks, however, due to the different of arguments between |
|
Thanks all for the comments and suggestions. It was very helpful to go through, and the package is definitely in a better place than before the review. I will now complete the tasks in #74 and release this version of the package.
Thank you for the feedback, I will take this into account when opening package reviews that are not an initial release, and will probably use full reviews more frequently than the release diff reviews.
Would still be interested to hear other's thoughts as well. |
This PR is to provide a platform to review the changes to the package since the last version release (v.0.1.0).
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.
🚨 This is the first time creating a PR from the HEAD of the
mainbranch to the commit tagged with the previous release. Therefore, if this PR has any problem or limitations, please let me know in order to make improvements in the future.