diff --git a/NAMESPACE b/NAMESPACE index 1362b15cd..b0165a2f1 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -95,6 +95,7 @@ importFrom(checkmate,check_atomic) importFrom(checkmate,check_data_frame) importFrom(checkmate,check_names) importFrom(checkmate,expect_class) +importFrom(checkmate,testInt) importFrom(checkmate,test_set_equal) importFrom(checkmate,test_subset) importFrom(checkmate,vname) diff --git a/R/epiprocess.R b/R/epiprocess.R index dd7df87a8..bea15b1d6 100644 --- a/R/epiprocess.R +++ b/R/epiprocess.R @@ -8,6 +8,7 @@ #' assert_logical assert_list assert_character assert_class #' assert_int assert_numeric check_data_frame vname check_atomic #' anyInfinite test_subset test_set_equal checkInt expect_class +#' testInt #' @importFrom cli cli_abort cli_warn #' @importFrom rlang %||% #' @name epiprocess diff --git a/R/grouped_epi_archive.R b/R/grouped_epi_archive.R index c6326751c..608ce27f5 100644 --- a/R/grouped_epi_archive.R +++ b/R/grouped_epi_archive.R @@ -204,10 +204,16 @@ ungroup.grouped_epi_archive <- function(x, ...) { #' @importFrom rlang !! !!! enquo quo_is_missing enquos is_quosure sym syms #' env missing_arg #' @export -epix_slide.grouped_epi_archive <- function(x, f, ..., before, ref_time_values, - time_step, new_col_name = "slide_value", - as_list_col = FALSE, names_sep = "_", - all_versions = FALSE) { +epix_slide.grouped_epi_archive <- function( + x, + f, + ..., + before = Inf, + ref_time_values = NULL, + new_col_name = "slide_value", + as_list_col = FALSE, + names_sep = "_", + all_versions = FALSE) { # Perform some deprecated argument checks without using ` = # deprecated()` in the function signature, because they are from # early development versions and much more likely to be clutter than @@ -231,7 +237,7 @@ epix_slide.grouped_epi_archive <- function(x, f, ..., before, ref_time_values, ", class = "epiprocess__epix_slide_all_rows_parameter_deprecated") } - if (missing(ref_time_values)) { + if (is.null(ref_time_values)) { ref_time_values <- epix_slide_ref_time_values_default(x$private$ungrouped) } else { assert_numeric(ref_time_values, min.len = 1L, null.ok = FALSE, any.missing = FALSE) @@ -247,19 +253,9 @@ epix_slide.grouped_epi_archive <- function(x, f, ..., before, ref_time_values, } # Validate and pre-process `before`: - if (missing(before)) { - cli_abort("`before` is required (and must be passed by name); - if you did not want to apply a sliding window but rather - to map `epix_as_of` and `f` across various `ref_time_values`, - pass a large `before` value (e.g., if time steps are days, - `before=365000`).") + if (!(anyInfinite(before) || testInt(before, lower = 0L, null.ok = FALSE, na.ok = FALSE))) { + cli_abort("`before` must be a non-negative integer or Inf.") } - before <- vctrs::vec_cast(before, integer()) - assert_int(before, lower = 0L, null.ok = FALSE, na.ok = FALSE) - - # If a custom time step is specified, then redefine units - - if (!missing(time_step)) before <- time_step(before) # Symbolize column name new_col <- sym(new_col_name) diff --git a/R/methods-epi_archive.R b/R/methods-epi_archive.R index 891cc064a..7a0334038 100644 --- a/R/methods-epi_archive.R +++ b/R/methods-epi_archive.R @@ -790,9 +790,8 @@ epix_slide <- function( x, f, ..., - before, - ref_time_values, - time_step, + before = Inf, + ref_time_values = NULL, new_col_name = "slide_value", as_list_col = FALSE, names_sep = "_", @@ -803,10 +802,16 @@ epix_slide <- function( #' @rdname epix_slide #' @export -epix_slide.epi_archive <- function(x, f, ..., before, ref_time_values, - time_step, new_col_name = "slide_value", - as_list_col = FALSE, names_sep = "_", - all_versions = FALSE) { +epix_slide.epi_archive <- function( + x, + f, + ..., + before = Inf, + ref_time_values = NULL, + new_col_name = "slide_value", + as_list_col = FALSE, + names_sep = "_", + all_versions = FALSE) { # For an "ungrouped" slide, treat all rows as belonging to one big # group (group by 0 vars), like `dplyr::summarize`, and let the # resulting `grouped_epi_archive` handle the slide: @@ -814,8 +819,7 @@ epix_slide.epi_archive <- function(x, f, ..., before, ref_time_values, group_by(x), f, ..., - before = before, ref_time_values = ref_time_values, - time_step = time_step, new_col_name = new_col_name, + before = before, ref_time_values = ref_time_values, new_col_name = new_col_name, as_list_col = as_list_col, names_sep = names_sep, all_versions = all_versions ) %>% diff --git a/man/epix_slide.Rd b/man/epix_slide.Rd index c8f09594b..f8e0b914f 100644 --- a/man/epix_slide.Rd +++ b/man/epix_slide.Rd @@ -10,9 +10,8 @@ epix_slide( x, f, ..., - before, - ref_time_values, - time_step, + before = Inf, + ref_time_values = NULL, new_col_name = "slide_value", as_list_col = FALSE, names_sep = "_", @@ -23,9 +22,8 @@ epix_slide( x, f, ..., - before, - ref_time_values, - time_step, + before = Inf, + ref_time_values = NULL, new_col_name = "slide_value", as_list_col = FALSE, names_sep = "_", @@ -36,9 +34,8 @@ epix_slide( x, f, ..., - before, - ref_time_values, - time_step, + before = Inf, + ref_time_values = NULL, new_col_name = "slide_value", as_list_col = FALSE, names_sep = "_", @@ -97,12 +94,6 @@ set to a regularly-spaced sequence of values set to cover the range of \code{version}s in the \code{DT} plus the \code{versions_end}; the spacing of values will be guessed (using the GCD of the skips between values).} -\item{time_step}{Optional function used to define the meaning of one time -step, which if specified, overrides the default choice based on the -\code{time_value} column. This function must take a positive integer and return -an object of class \code{lubridate::period}. For example, we can use \code{time_step = lubridate::hours} in order to set the time step to be one hour (this -would only be meaningful if \code{time_value} is of class \code{POSIXct}).} - \item{new_col_name}{String indicating the name of the new column that will contain the derivative values. Default is "slide_value"; note that setting \code{new_col_name} equal to an existing column name will overwrite this column.} @@ -124,6 +115,12 @@ from \code{new_col_name} entirely.} \code{ref_time_value - before} and \code{ref_time_value}. Otherwise, \code{f} will be passed only the most recent \code{version} for every unique \code{time_value}. Default is \code{FALSE}.} + +\item{time_step}{Optional function used to define the meaning of one time +step, which if specified, overrides the default choice based on the +\code{time_value} column. This function must take a positive integer and return +an object of class \code{lubridate::period}. For example, we can use \code{time_step = lubridate::hours} in order to set the time step to be one hour (this +would only be meaningful if \code{time_value} is of class \code{POSIXct}).} } \value{ A tibble whose columns are: the grouping variables, \code{time_value}, diff --git a/tests/testthat/test-epix_slide.R b/tests/testthat/test-epix_slide.R index a5b72cbfa..8e75fa9bf 100644 --- a/tests/testthat/test-epix_slide.R +++ b/tests/testthat/test-epix_slide.R @@ -174,30 +174,18 @@ test_that("epix_slide works as intended with `as_list_col=TRUE`", { }) test_that("epix_slide `before` validation works", { - expect_error( - xx %>% epix_slide(f = ~ sum(.x$binary)), - "`before` is required" - ) expect_error( xx %>% epix_slide(f = ~ sum(.x$binary), before = NA), - "Assertion on 'before' failed: May not be NA" + "`before` must be a non-negative integer or Inf." ) expect_error( xx %>% epix_slide(f = ~ sum(.x$binary), before = -1), - "Assertion on 'before' failed: Element 1 is not >= 0" + "`before` must be a non-negative integer or Inf." ) expect_error( xx %>% epix_slide(f = ~ sum(.x$binary), before = 1.5), - regexp = "before", - class = "vctrs_error_incompatible_type" - ) - # We might want to allow this at some point (issue #219): - expect_error( - xx %>% epix_slide(f = ~ sum(.x$binary), before = Inf), - regexp = "before", - class = "vctrs_error_incompatible_type" + "`before` must be a non-negative integer or Inf." ) - expect_error(xx %>% epix_slide(f = ~ sum(.x$binary)), "`before` is required") # These `before` values should be accepted: expect_error( xx %>% epix_slide(f = ~ sum(.x$binary), before = 0),