Skip to content
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

Full package review for {bpmodels} CRAN release #79

Closed
wants to merge 267 commits into from

Conversation

jamesmbaazam
Copy link
Collaborator

@jamesmbaazam jamesmbaazam commented Jun 21, 2023

This is a full package review in preparation for submitting {bpmodels} to CRAN. It would be helpful to provide comments that help resolve the checklist provided here #80.

Modalities

How to become a full reviewer

Self-assignment under the "Reviewers" tab on the right side of this window.

Required number of reviewers

Maximum of 2. PS: Don't self-assign when this is reached.

Full reviewers

  1. @joshwlambert
  2. @pratikunterwegs

Deadline

Not currently set.

What next?

  • This PR will be closed when all relevant issues have been resolved.
  • Closing this PR will not close any issues raised but will serve as a reference for raising issues to be resolved in subsequent pull requests.

sbfnk and others added 30 commits January 16, 2019 09:52
Growing a vector in R tends to be slow. This process will run faster due to the pre-allocation:

```r
Unit: relative
                                                                  expr      min       lq     mean   median       uq      max neval cld
            {     slow <- c()     for (i in seq(1e+06)) slow[i] <- 1 } 5.156572 5.071423 5.084367 5.023986 5.271332 3.261024   100   b
 {     fast <- integer(1e+06)     for (i in seq(1e+06)) fast[i] <- 1 } 1.000000 1.000000 1.000000 1.000000 1.000000 1.000000   100  a
```
@jamesmbaazam
Copy link
Collaborator Author

  • I would reword "The negative binomial distribution is commonly used in epidemiology to account for individual variation in transmissibility, also known as superspreading". As small amounts of variation (e.g. poisson or geometric) are not considered superspreading.

Not sure I understand how your second statement relates to the first.

@jamesmbaazam
Copy link
Collaborator Author

  • It seems that in figure 1 of the COVID vignette many of the grey lines are above the red median line. I would have expected the red line to sit in the middle of each stochastic simulation?

That's a good point. I'll crosscheck that.

@jamesmbaazam
Copy link
Collaborator Author

  • It might be good to have a control on simulating negative serial intervals. For now you could limit the functionality to work with strictly positive serial interval.

I am not sure that this is a necessary feature as we do not want to restrict the user on what types of serial intervals to simulate. There's a case for users wanting to simulate negative serial intervals to pursue questions about the infector being infectious before developing symptoms (See the "serial interval" section of this paper that for example mentions that observed serial intervals can be negative, and [this one](https://www.ncbi.nlm.nih.gov/pmc/articles/PMC7239082/#:~:text=Negative%20serial%20intervals%20(left%20of,asymptomatic%20or%20mildly%20symptomatic%20cases) that observed negative serial intervals, and other examples in the literature.

@sbfnk Thoughts on this requested feature?

@jamesmbaazam
Copy link
Collaborator Author

  • The code chunk
chain_sizes <- c(1, 1, 4, 7) # example of observed chain sizes
chain_ll(chain_sizes, "binom", "size", size = 1, prob = 0.5,
         nsim_offspring = 100)

returns -Inf is this correct?
Could it be because you're using the binomial instead of the negative binomial?

There is an analytical solution for the negative binomial size distribution. This code chunk demos the simulated binomial size result.

@sbfnk
Copy link
Collaborator

sbfnk commented Sep 1, 2023

  • In the get started vignette, is a vector of four chains sizes a good example to use? I don't know, but I would imagine the power to differentiate between generating models is low when the sample size is four (assuming that the sample size is the number of transmission chains and not the total size of all chains)

It's a good point -

  • It might be good to have a control on simulating negative serial intervals. For now you could limit the functionality to work with strictly positive serial interval.

I am not sure that this is a necessary feature as we do not want to restrict the user on what types of serial intervals to simulate. There's a case for users wanting to simulate negative serial intervals to pursue questions about the infector being infectious before developing symptoms (See the "serial interval" section of this paper that for example mentions that observed serial intervals can be negative, and [this one](https://www.ncbi.nlm.nih.gov/pmc/articles/PMC7239082/#:~:text=Negative%20serial%20intervals%20(left%20of,asymptomatic%20or%20mildly%20symptomatic%20cases) that observed negative serial intervals, and other examples in the literature.

@sbfnk Thoughts on this requested feature?

I agree - supporting negative SIs is a feature not a bug.

@jamesmbaazam
Copy link
Collaborator Author

All issues raised through this full package review have been addressed and logged as GitHub issues, so I am closing this PR now.

@jamesmbaazam jamesmbaazam deleted the full_package_review branch October 6, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants