Slather access workflow and test data & update layer_add_target_date() and layer_add_forecast_date() accordingly
#220
Conversation
rachlobay
changed the title
layer_add_target_date() and layer_add_forecast_date()` accordinglylayer_add_target_date() and layer_add_forecast_date() accordingly
| components$mold <- workflows::extract_mold(object) | ||
| components$forged <- hardhat::forge(new_data, | ||
| blueprint = components$mold$blueprint) | ||
| components$keys <- grab_forged_keys(components$forged, | ||
| components$mold, new_data) | ||
| components <- apply_frosting(object, components, the_fit, the_recipe, ...) | ||
| components <- apply_frosting(object, components, new_data, ...) |
There was a problem hiding this comment.
Don't we still need the_fit and the_recipe? Am I missing something in the changes?
There was a problem hiding this comment.
The epi_workflow (object) contains the recipe and fit, so it seemed unnecessary to include them separately.
There was a problem hiding this comment.
We might need some more tests then. I would think this would fail for sure.
I see now. You modified it in apply_frosting.epi_workflow().
For the future, when PRs touch lots of files like this one, it's helpful for me if your initial comment goes through the logic of the things you do. That way I can figure out where to look, and if there are simplifications that happen across multiple files, it's easier for me to understand the idea.
|
Yes. Good catch for 2! I didn't think to look in inst/templates. So I've updated that, but I don't see 1 (the signature in slather.layer_unnest looks ok, but maybe something inside the function is off? Let me know). |
|
Huh. For some reason, I didn't see the Merge when ready. |
Fixes #108 and #109 by adding
workflowandnew_datato the signature of slather. Then used themax(new_data$time_value)as the default forecast date (and that + ahead for the default target date). Finally, updated the tests and documentation to reflect these new defaults.