Skip to content

Created a preprocessing step that limits the size of the training window #53

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

Merged
merged 37 commits into from
Aug 29, 2022

Conversation

rachlobay
Copy link
Contributor

@rachlobay rachlobay commented Jun 17, 2022

Addressed issue 36 by creating a preprocessing step, step_training_window(), that limits the size of the training window to the n most recent observations per location. See training_window.R and test-step_training_window.R for some tests.

@rachlobay rachlobay requested a review from brookslogan June 17, 2022 23:02
@rachlobay rachlobay linked an issue Jun 17, 2022 that may be closed by this pull request
@rachlobay rachlobay marked this pull request as ready for review June 17, 2022 23:52
@rachlobay rachlobay requested a review from dajmcdon as a code owner June 17, 2022 23:52
@dajmcdon
Copy link
Contributor

It looks like this is failing because of cmu-delphi/epiprocess#96. Blocked for the moment.

@rachlobay
Copy link
Contributor Author

rachlobay commented Jun 29, 2022

@dajmcdon, when using training data in bake() (that is, bake(new_data = NULL) such as in the tests for this function in particular, we now lose the epi_df class of the resulting data frame. In that case, the baked data gets demoted to a plain old tibble.

I think the issue here is that when we put new_data = NULL in bake, bake.recipe sends us to juice() and since the composition is a tibble (by default), then new_data <- tibble::as_tibble(new_data) on line 845-ish of juice() here: https://github.com/tidymodels/recipes/blob/main/R/recipe.R. This is why I think that all was well with the class when I used bake(new_data = NULL) along w/that old as_tibble.epi_df function that was removed… What do you think?

@dajmcdon
Copy link
Contributor

I suspect you're right. We're going to need a new bake.epi_recipe() method. Ugh.

@dajmcdon dajmcdon merged commit 714b972 into frosting Aug 29, 2022
@dajmcdon dajmcdon deleted the 36-step_training_window branch August 29, 2022 15:45
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.

step_training_window
2 participants