-
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
add forecast_opts() #901
add forecast_opts() #901
Conversation
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8003d06 is merged into main:
|
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.
Thanks for implementing this, Seb.
I made a few comments and suggestions.
Co-authored-by: James Azam <[email protected]>
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.
Thanks, @jamesmbaazam. Did the new identification of gaps look OK to you (I think you weren't convinced of the idea in the corresponding Issue)?
Line 447 in 8003d06
if (length(gaps) == 1 && gaps > 1) { ## all gaps are the same |
That would be better too. |
I like the current implementation. |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if f0f453e is merged into main:
|
Description
This PR closes #867 and part of #898 whilst enabling addressing #640.
It introduces a
forecast
argument toestimate_infections()
that is set viaforecast_opts()
(preferred overadd_horizon()
as it allows us to identify the last date in the data - though theadd_horizon()
function exists internally so we could make the switch at a later time if we want).Initial submission checklist
devtools::test()
anddevtools::check()
).devtools::document()
).lintr::lint_package()
).After the initial Pull Request