cmu-delphi / epipredict

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

refactor: epi_recipe only accepts epi_df #350

Closed dshemetov closed 1 week ago

dshemetov commented 1 week ago

Checklist

Please:

Change explanations for reviewer

Only allows epi_df in epi_recipe and everything else just gives a clear error as such. Open to conversation about this, I don't know if falling back to regular recipes is useful sometimes, I haven't needed that functionality, but if it's not, then this is the behavior I prefer.

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

dajmcdon commented 1 week ago

I'm working on a more complicated refactor of this, but the short version is I don't think this is the right behaviour. An epi_recipe should be a subclass of recipe (it isn't currently) and operate on non-epi_df.

In the meantime, if you want it to warn on epi_recipe.default() that's fine, but it should produce a recipe.

A user who downloads our package shouldn't be prevented from recipe behaviour just because they started with the wrong function.

dshemetov commented 1 week ago

@dajmcdon Cool, I switched this change over to a warning instead.