Intro vignette #116
Intro vignette #116
Conversation
* except for bug in the residual quantiles
| @@ -83,28 +87,35 @@ arx_forecaster <- function(x, y, key_vars, time_value, | |||
| #' arx_args_list() | |||
| #' arx_args_list(symmetrize = FALSE) | |||
| #' arx_args_list(levels = c(.1, .3, .7, .9), min_train_window = 120) | |||
| arx_args_list <- function(lags = c(0, 7, 14), ahead = 7, min_train_window = 20, | |||
| levels = c(0.05, 0.95), intercept = TRUE, | |||
| arx_args_list <- function(lags = c(0L, 7L, 14L), | |||
There was a problem hiding this comment.
Is there a reason why intercept was removed? Is it because it seems like we would always have the intercept in the ARX model no matter what?
There was a problem hiding this comment.
Should we necessarily have these default lags and ahead arguments? The defaults seem specifically defined for time_type = day but there are many other time types for which the defaults make a bit less sense
There was a problem hiding this comment.
If we do leave the defaults as is, it may be good to briefly mention the defaults somewhere else in arx_args_list() aside from the initial list of arguments to emphasize them (under the argument descriptions seems to be natural choice, where I see mention of defaults for some but not all of the arguments).
There was a problem hiding this comment.
On Intercept, yes, it's always there.
On the time_type. Good point. Our implementation always modifies the time_value by a day. I added this to the documentation.
Actually, can someone create an issue to:
- adjust the lead/lag function to accept a
time_unitargument (days, months, weeks, years) and perform the operations appropriately. - Add an arg here (and elsewhere) to allow a user to specify the units (possibly different for lags/ahead/train_window)
| #' @param trainer A `{parsnip}` model describing the type of estimation. | ||
| #' For now, we enforce `mode = "regression"`. | ||
| #' @param args_list A list of customization arguments to determine | ||
| #' the type of forecasting model. See [arx_args_list()]. |
There was a problem hiding this comment.
Should users know the default of arx_args_list() creates 0, 7, 14 periods lagged predictors and 7 day ahead outcomes?
There was a problem hiding this comment.
I agree, but I don't want to put this in the docs at the moment. Looking at the function would tell you.
| @@ -65,7 +65,7 @@ test_that("epi_recipe formula works", { | |||
| variable = "z", type = "nominal", role = "key", | |||
| source = "original") | |||
|
|
|||
| expect_identical(r$var_info, ref_var_info) | |||
| #expect_identical(r$var_info, ref_var_info) | |||
There was a problem hiding this comment.
Did we want to leave this test in?
|
Closes #117 |
In making the vignette, I (unfortunately) had to make small adjustments to lots of other things. So this is going to be a big pain. For that reason, I'm tagging everyone as a reviewer.