Closed pratikunterwegs closed 2 years ago
Hi @thimotei and @joshwlambert, I've requested that you review this PR. The idea is 1. to actually find things to fix and improve before merging into main
, but also 2. to familiarise you with code review, and with the finalsize
package.
See a little discussion on code review for PRs here: https://github.com/epiverse-trace/blueprints/issues/6#issuecomment-1245587468
The PR references a number of issues, and the main comment above lays out a summary of the PR, and how it addresses the linked issues. Hopefully this is informative - if not, add a comment in the thread. If code in any of the files is unclear, add a comment on that chunk, which will open a conversation, to which I will reply.
If you can think of an easy fix or improvement, make a suggestion for an edit, which I can look at and potentially accept as is; this will make you co-authors on the PR.
If you have any questions about reviewing, you can ask me, @Bisaloo or @thibautjombart.
I've had a quick skim - will leave a more thorough review to others. Minor comments above. A couple of general points:
I've had a quick skim - will leave a more thorough review to others. Minor comments above. A couple of general points ...
Thanks @TimTaylor! Will work on the vectorisation and treating 1D matrices as vectors - as you noticed it's a direct translation from the Cpp - could def be something to be gained there.
Only a couple of very minor points raised in the code. Overall, code is clean and works as expected. I have not fully analysed the details of the model but rather the code itself. Happy to approve. Nice work @pratikunterwegs.
Only a couple of very minor points raised in the code. Overall, code is clean and works as expected. I have not fully analysed the details of the model but rather the code itself. Happy to approve. Nice work @pratikunterwegs.
Thanks again @joshwlambert, and thanks for catching the superfluous test file and for noticing that pi
overwrites the inbuilt value. I've changed that to something more informative as well. I'll keep this open until Monday morning, then merge it. Perhaps we can go over rebasing the feature/susc_grps
branch on main after this merge, that should be a challenge. :)
Adds functionality that allows final sizes in demographic groups with heterogeneous mixing and heterogeneous susceptibility. In short, demographic groups can be spread into discrete 'risk groups' (or susceptibility groups), each of which has a (potentially) different susceptibility to the epidemic. This fixes #41.
This PR adds the function
final_size_grps
which fixes #45, which has the following additional arguments:susceptibility
: A matrix of susceptibility to the epidemic, where matrix element[i,j]
represents the susceptibility of demographic group i in risk group j.p_susceptibility
: A matrix representing the proportion of (or probability of) demographic group i being in risk group j.Includes two internal functions,
epi_spread
andsolve_final_size_iterative
, which correspond to internal functions in #39. This code is essentially an R implementation of https://gitlab.com/epidemics-r/code_snippets/-/blob/feature/newton_solver/include/finalsize.hpp#L20 etc. These fix #44.Also includes more tests for all three functions; fixes #46, and fixes #47. In general, input checking is restricted to top level functions exposed to the user, while unit tests mostly target internal functions (e.g. checking whether final size per group is in the range [0, 1]). Tests are based on https://gitlab.com/epidemics-r/code_snippets/-/blob/feature/newton_solver/tests/catch_final_size.cpp. Documentation added fixes #48.