Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add the 7dav we talked about along with the std #76
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
Add the 7dav we talked about along with the std #76
Changes from 13 commits
e4b3d45
4a78810
1edc6f6
e63be89
ea335c9
8df5f05
36608f6
0eb61e3
c50249b
0b16c81
514219e
b976103
c4c7430
d6d4cdd
d542be2
16dd527
4b34428
af3ff95
ebb9db3
15d35d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: why do we want to clear
NA
s? (Probably should add an explanatory comment. Note this is sort of an "anti-complete()
" so even if we wantcomplete()
d stuff anywhere below then those things need to be careful. E.g., slide computation passed toepi_slide()
should probably testnrow(.x)
to decide whether it should output NA.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lastminute here means
NA
in the final days. Epipredict just chokes on that, when it should be treating theNA
's as a latency adjustment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that makes sense, though the current
clear_lastminute_nas
clears all NAs not just lastminute ones. Relates to lastminute-imputation & gap treatment stuff. I think we discussed this elsewhere but current approach probably works okay on hhs hosp data (and hopefully any chng data used?? idk there), but when adding to epipredict should probably be a bit more careful since it could be used on more problematic data sets.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Do we always want to do this?
E.g., dev10 pairs lags of separate rolling means of the original with the sd estimate.[nvm, this is what seems to be happening here as well, though I need to check where the lags are specified] And how are the requested lags derived for these predictors? May make more sense to just keep everything and have the model specify what it wants. Or, in epipredict framework, have a smooth mean step that adds (lags of) smoothed means & assigns predictor roles, and sd step that works in a similar way, so dev10 could be a step_epi_lag + step_rolling_sd, while the implied behavior above could be a step_rolling_mean + step_rolling_sd. (thought: I'm wishing for astep_epi_slide
and astep_mutate
that allow.by
, but these concrete steps would be good too I think. And same thing inepiprocess
, probably want specific rolling mean and rolling sd helpers..)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lags are applied to all predictors, and are specified per-model in the
...
, which can be any of the arx args.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we may want to apply different lagsets to the 7dav (7*(0:4) ish) and the sd covariate (maybe just 0) & ignore the original non-7dav'd predictors. Is that possible via the list-of-vectors form of
lags
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what the
update_predictors
function is for. It drops the non-7dav'd versions from the list of predictors.Apparently, though I haven't actually tried it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the current validation will probably complain if we try it.
issue:
c(smooth_cols, sd_cols)
is probably wrong when exactly one ofsmooth_cols
andsd_cols
isNULL
. These variables have not been overwritten withget_trainable_names
in this function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (with
arx_postprocess
...): ignoresforecast_date
passed through...
; doesn't respecttarget_date
passed through...
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's intended behavior. Both of those are set through adjusting
ahead
fortarget_date
and theas_of
ofepi_data
for theforecast_date
.Yeah, I should probably just completely refactor the way I wrote
update_predictors
. I think it should work for now though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think a quick fix here would be to just make sure
forecast_date
andtarget_date
areNULL
inargs_list
. Or to move away from...
forwarding to manually handling/forwarding/forbidding each parameter. I assume you're not using passing in either of these parameters downstream, so maybe this could also be deferred to a separate Issue.Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.