-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add network model simulation #60
Conversation
…ew arguments (mean_contacts, contact_interval, prob_infect)
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.
As far as I could see the function has the expected functionality - just a couple of minor comments.
My main comment relates to the design, and I'm not yet quite convinced of the new approach. It would be fairly straightforward to add the network bit to {epichains}
, or even to use the existing {epichains}
with a tweaked offspring distribution (although that would be inefficient). There are two reasons that I think updating {epichains}
and using this is preferable to implementing a separate model here: 1) The code here would benefit from any current/future updates to {epichains}
(ones that might already be useful here are: limiting simulations to given maximum and susceptible depletion in finite populations) rather than having to replicate any of them here. 2) The {epichains}
package itself would benefit from these additions, which could be something like a create_network_offspring_dist()
function that takes a contact distribution and turns it into an offspring distribution (a few lines), and a probability of infection per offspring, which is already implemented for susceptible depletion so would just need change in one line and addition of an argument.
Along the same lines I also think it would be good to let the user choose whether they want a static network (with contact distribution and contact interval) or random (with offspring distribution and generation interval, i.e. the previous version) realisation rather than only allowing one or the other - as stated in https://github.com/epiverse-trace/simulist/pull/60/files#r1466717823, they're both sensible and depending on the situation either might be preferred.
Only other comment is that the new tests don't check for model correctness, just functions returning correct types etc. It would be good to add something that makes sure that future changes don't affect model results.
Thanks for your comments. I am working through each of them and will apply the updates to this PR before merging. @sbfnk on the point about {epichains}, I completely agree with everything you've stated. The long-term plan will be for this code to live somewhere else (likely {epichains}) and whichever package hosts that code can become a dependency of {simulist} (assuming it is on CRAN if {simulist} is). However, I don't see this limiting short-term development. What I mean is that, in my opinion, we can merge this PR once we're happy with the updates and release the package, and the code can be ported in a future version. Waiting for the packages to align enough and move the code now would slow down development, but as always happy to discuss. |
I'm slightly unsure how to proceed with these comments. I plan to add an option to the However, the second comment seems different as the contact distribution would become the offspring distribution when the probability of infection is 1, and to me (this is where I am confused) is not related to the network effect (excess degree distribution). If we were to offer both arguments for contact and offspring distribution it complicates the signature of the functions, and instead I would recommend just documenting somewhere that when |
I'd agree that having |
…dd_names argument
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
Thanks, I've now used snapshot testing to test both the structure and values output by |
I've finished the changes in this PR. I will merge at the end of the day and work through the other issues towards a version release. Please let me know if there are any aspects of this PR, especially changes since the last round of comments, that you would like changing before merging. |
This PR addresses and closes #58. It replaces the single-type branching process model (
bpmodels::chain_sim()
) and a subsequent contact distribution sampling (which were independent, see issue #35), with a single simulation process that keeps track of infectious individuals and contacts of infectious individuals and correctly samples taking into account a network effect. The new functionality is in.sim_network_bp()
.The new functionality comes with breaking changes for the exported simulation functions. The arguments supplied to the
sim_*()
functions was:R
(reproduction number),serial_interval
andcontact_distribution
, the arguments now required arecontact_distribution
(now correctly implemented as the number of contacts per infected individual, no longer the contacts that are not infected),contact_interval
(replacing serial interval, as the time delays also include time between contacts that are not infected),prob_infect
(probability of infection per contact).Documentation and tests have been updated to use these new arguments.
Another breaking change is the
<data.frame>
output bysim_contacts()
now has the column headingsage
andgender
instead ofcnt_age
andcnt_gender
. This now matches the output ofsim_linelist()
which already had column namesage
andgender
.Some of the internal functions have been updated to accommodate the new data being produced by
.sim_network_bp()
.The dependency on {bpmodels} is removed. The CITATION.cff file is update as a result.
This PR also closes #57 by setting a seed in the README.