-
Notifications
You must be signed in to change notification settings - Fork 33
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
base: main
Are you sure you want to change the base?
Conversation
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.
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
Ah touchstone and as CRAN check are just having a fail party everywhere |
Oh yes that would be great! |
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. |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 746c3ba is merged into main:
|
Hmm, looks like we get a slowdown and fitting failure(s). This may need more exploration. |
Description
This PR closes #902 by
prior_infections
to be based on the intercept of the linear model instead of mean of first week, if availableseeding_time
if greater than 7 (as that is the period for which exponential growth is assumed in the model)initial_infections
byfrac_obs
before modelling initial exponential growthinitial_infections
to be approximately PoissonAs a result we've go from
to
in the example mentioned in the Issue - see
EpiNow2/inst/dev/recover-synthetic/rt.R
Line 14 in dbb6536
More generally the values of
initial_growth
/initial_infections
no longer seem to affectfrac_obs
, at the expense of (expected) correlation between the initial estimates.Initial submission checklist
devtools::test()
anddevtools::check()
).devtools::document()
).lintr::lint_package()
).After the initial Pull Request