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

improve early calculation #903

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

improve early calculation #903

wants to merge 8 commits into from

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Dec 19, 2024

Description

This PR closes #902 by

  • changing the calculation of prior_infections to be based on the intercept of the linear model instead of mean of first week, if available
  • fitting the initial model for seeding_time if greater than 7 (as that is the period for which exponential growth is assumed in the model)
  • dividing initial_infections by frac_obs before modelling initial exponential growth
  • changing the prior for initial_infections to be approximately Poisson

As a result we've go from

image

to

image

in the example mentioned in the Issue - see

init <- estimate_infections(example_confirmed[1:100],

More generally the values of initial_growth / initial_infections no longer seem to affect frac_obs, at the expense of (expected) correlation between the initial estimates.

Initial submission checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have tested my changes locally (using devtools::test() and devtools::check()).
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required and rebuilt docs if yes (using devtools::document()).
  • I have followed the established coding standards (and checked using lintr::lint_package()).
  • I have added a news item linked to this PR.

After the initial Pull Request

  • I have reviewed Checks for this PR and addressed any issues as far as I am able.

Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really great. Not quite understanding the CI fail for as CRAN (requiring future) and haven't checked out the benchmarks. Really good this has solved the correlation we were seeing

noting my preference would be we further explore using the approach from @SamuelBrand1 used in EpiAware where we solve for the implied growth rate based on the generation time and initial Rt estimate. I can knock something up for this (it will look something like this: https://github.com/CDCgov/Rt-without-renewal/blob/0d4095ecf5d67b599b26a8822679adebb40b164a/EpiAware/src/EpiInfModels/utils.jl#L44 but I will probably use the algebraic system solver for simplicity) in a future PR

@seabbs
Copy link
Contributor

seabbs commented Dec 19, 2024

Ah touchstone and as CRAN check are just having a fail party everywhere

@sbfnk
Copy link
Contributor Author

sbfnk commented Dec 20, 2024

noting my preference would be we further explore using the approach from @SamuelBrand1 used in EpiAware where we solve for the implied growth rate based on the generation time and initial Rt estimate.

Oh yes that would be great!

@seabbs seabbs enabled auto-merge December 20, 2024 14:34
@SamuelBrand1
Copy link

This looks really great. Not quite understanding the CI fail for as CRAN (requiring future) and haven't checked out the benchmarks. Really good this has solved the correlation we were seeing

noting my preference would be we further explore using the approach from @SamuelBrand1 used in EpiAware where we solve for the implied growth rate based on the generation time and initial Rt estimate. I can knock something up for this (it will look something like this: CDCgov/Rt-without-renewal@0d4095e/EpiAware/src/EpiInfModels/utils.jl#L44 but I will probably use the algebraic system solver for simplicity) in a future PR

Yeah, I did it this way because I didn't want to worry about AD through a zero finder so hand rolled a few steps of a Newton solver. Not optimal, I like the look of the algebraic system solver.

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 746c3ba is merged into main:

  • ✔️default: 20.2s -> 31.2s [-12.76%, +120.57%]
  • ✔️no_delays: 25.1s -> 26.8s [-6.46%, +19.85%]
  • ❗🐌random_walk: 9.19s -> 12.9s [+33.4%, +47.23%]
  • ✔️stationary: 16s -> 18.4s [-49.62%, +78.82%]
  • ❗🐌uncertain: 34.1s -> 41.1s [+11.11%, +30.01%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@sbfnk
Copy link
Contributor Author

sbfnk commented Dec 20, 2024

Hmm, looks like we get a slowdown and fitting failure(s). This may need more exploration.

@sbfnk sbfnk disabled auto-merge December 20, 2024 17:30
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.

Recovery of frac_obs / priors for initial infections and growth
3 participants