cmu-delphi / epipredict

Tools for building predictive models in epidemiology.
https://cmu-delphi.github.io/epipredict/
Other
8 stars 8 forks source link

feat: add forecast method #319

Closed dshemetov closed 2 months ago

dshemetov commented 2 months ago

Checklist

Please:

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:

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

dshemetov commented 2 months ago

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.

dajmcdon commented 2 months ago

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 commented 2 months ago

Made #323 to track those

dajmcdon commented 2 months ago

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

dshemetov commented 2 months ago

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

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.