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

feat: add forecast method #319

Merged
merged 4 commits into from
Apr 30, 2024
Merged

feat: add forecast method #319

merged 4 commits into from
Apr 30, 2024

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Apr 12, 2024

Checklist

Please:

  • Make sure this PR is against "dev", not "main".
  • Request a review from one of the current epipredict main reviewers:
    dajmcdon.
  • Make sure to bump the version number in DESCRIPTION and NEWS.md.
    Always increment the patch version number (the third number), unless you are
    making a release PR from dev to main, in which case increment the minor
    version number (the second number).
  • Describe changes made in NEWS.md, making sure breaking changes
    (backwards-incompatible changes to the documented interface) are noted.
    Collect the changes under the next release number (e.g. if you are on
    0.7.2, then write your changes under the 0.8 heading).
  • Consider pinning the epiprocess version in the DESCRIPTION file if
    • You anticipate breaking changes in epiprocess soon
    • You want to co-develop features in epipredict and epiprocess

Change explanations for reviewer

A very simple PoC we discussed adding the forecast function. Not sure if I stored the train_data in the right place, currently in epi_workflow$fit$meta$train_data, hopefully won't clobber anything there. Might be able to use ... instead of duplicating get_train_data args (though might be moot if we're aiming to deprecate that function anyway).

The changes in _pkgdown.yml are mostly whitespace changes from my YAML linter (it likes that indent setting I guess). The only actual change was adding the forecast function to the list. I thought about doing the whole dispatch thing with forecast.epi_workflow, but it seemed like extra boilerplate. I could see it being handy if we expect forecast to be a clobberable function name from other packages.

TODO:

  • more tests, with other arguments
  • replace uses of predict in vignettes and examples with forecast

Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch

@dshemetov dshemetov force-pushed the ds/forecast branch 2 times, most recently from 19039b5 to 7bf677b Compare April 12, 2024 23:35
R/epi_workflow.R Outdated Show resolved Hide resolved
R/epi_workflow.R Outdated Show resolved Hide resolved
R/epi_workflow.R Outdated Show resolved Hide resolved
@dshemetov dshemetov force-pushed the ds/forecast branch 2 times, most recently from edb2d22 to 934e7bf Compare April 15, 2024 19:09
@dshemetov dshemetov requested a review from dajmcdon April 15, 2024 19:23
Copy link
Contributor

@dajmcdon dajmcdon left a comment

Choose a reason for hiding this comment

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

  • Major change: forecast needs to be a method, not a standalone.
  • Minor issue: we should perhaps think about how many of the tests should be pivoted. I'm not sure it's a bad idea to duplicate or keep some as is to ensure that both options continues to work going forward.

R/arx_forecaster.R Outdated Show resolved Hide resolved
R/epi_workflow.R Outdated Show resolved Hide resolved
R/flatline_forecaster.R Show resolved Hide resolved
R/layer_cdc_flatline_quantiles.R Outdated Show resolved Hide resolved
_pkgdown.yml Outdated Show resolved Hide resolved
tests/testthat/test-population_scaling.R Outdated Show resolved Hide resolved
vignettes/epipredict.Rmd Outdated Show resolved Hide resolved
vignettes/epipredict.Rmd Outdated Show resolved Hide resolved
@dshemetov
Copy link
Contributor Author

dshemetov commented Apr 24, 2024

Thanks for the review. Addressed your comments and the major point about forecast. Thinking we should handle the suppressWarning thing somewhere else, it seems to be kind of a rabbit hole (the answer isn't an easy "yea, no more warnings").

As for the tests comment, there are still many tests that I did not pivot over and those were the ones that used a non-standard new_data setting. I also have a test to compare the output of a forecast() call with an equivalent predict() call. So I don't feel like predict() is losing much in test coverage.

@dshemetov dshemetov requested a review from dajmcdon April 24, 2024 21:37
@dajmcdon
Copy link
Contributor

On the warnings, it used to be that running a forecaster would always warn (because the data had as_of sometime in the past). I removed that warning. But all these others should be fixed. Agreed out of scope, but now that you've identified the problematic ones, can you create an issue for them?

@dshemetov
Copy link
Contributor Author

Made #323 to track those

Copy link
Contributor

@dajmcdon dajmcdon left a comment

Choose a reason for hiding this comment

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

Just a small suggestion for the method itself. Hopefully, you can just accept (assuming you agree) and CI will still pass.

R/epi_workflow.R Show resolved Hide resolved
R/epi_workflow.R Outdated Show resolved Hide resolved
* check postprocessor for forecast_date in forecast.epi_workflow
* add test
@dshemetov dshemetov merged commit 5e50a5a into dev Apr 30, 2024
3 checks passed
@dshemetov dshemetov deleted the ds/forecast branch April 30, 2024 23:13
@dajmcdon
Copy link
Contributor

dajmcdon commented May 2, 2024

@dshemetov what is "partial" about the fix to #293?

@dshemetov
Copy link
Contributor Author

In my notes from our last sync, we planned 3 PRs related to the discussions in that issue:

  • Add a forecast() function (this PR)
  • Rewrite the get_test_data() function (tbd)
  • Change predict to “join new data onto old and forecast” (tbd)

I'm happy to tackle the latter two in the coming months, though we'll probably need to sync and clarify how we're going to tackle those.

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

2 participants