Replaced n in epi_slide and epix_slide
#203
Conversation
…km-fix-epix_slide
Merge branch 'main' of https://github.com/cmu-delphi/epiprocess into km-fix-epix_slide # Conflicts: # R/methods-epi_archive.R # man/epix_slide.Rd # tests/testthat/test-methods-epi_archive.R
…st for testing negative integers later.
Merge branch 'main' of https://github.com/cmu-delphi/epiprocess into km-fix-epix_slide # Conflicts: # R/archive.R # R/methods-epi_archive.R # man/epi_archive.Rd # man/epix_slide.Rd
Merge branch 'km-slide-n-replace2.1' of https://github.com/dajmcdon/epiprocess into km-fix-epix_slide # Conflicts: # man/epi_slide.Rd # man/epix_slide.Rd # vignettes/advanced.Rmd # vignettes/archive.Rmd
|
Note: I can do a new one without those changes from #188 if needed. However, both of these are similar to each other. |
R/archive.R
Outdated
| if (missing(ref_time_values)) { | ||
| ref_time_values = unique(self$DT$time_value) | ||
| if (missing(ref_versions)) { | ||
| ref_versions = unique(self$DT$version) |
There was a problem hiding this comment.
Based on discussion with Ryan, we shouldn't rename ref_time_values (yet, at least).
R/archive.R
Outdated
| @@ -584,23 +584,24 @@ epi_archive = | |||
| #' details. | |||
| #' @importFrom data.table key | |||
| #' @importFrom rlang !! !!! enquo quo_is_missing enquos is_quosure sym syms | |||
| slide = function(f, ..., n, group_by, ref_time_values, | |||
| slide = function(f, ..., max_version_gap, group_by, | |||
| ref_versions, | |||
There was a problem hiding this comment.
Based on discussion with Ryan, max_version_gap should be called before.
R/archive.R
Outdated
| @@ -619,7 +620,7 @@ epi_archive = | |||
| # Computation for one group, one time value | |||
| comp_one_grp = function(.data_group, | |||
| f, ..., | |||
| time_value, | |||
| version, | |||
There was a problem hiding this comment.
In line with the above, please call this ref_time_value.
R/archive.R
Outdated
| @@ -665,21 +666,22 @@ epi_archive = | |||
|
|
|||
| # Note that we've already recycled comp value to make size stable, | |||
| # so tibble() will just recycle time value appropriately | |||
| return(tibble::tibble(time_value = time_value, | |||
| return(tibble::tibble(version = version, | |||
There was a problem hiding this comment.
Again, we've put such changes on hold / are still discussing; this should be time_value = ref_time_value. (#163)
R/archive.R
Outdated
| if (rlang::is_formula(f)) f = rlang::as_function(f) | ||
|
|
||
| x = purrr::map_dfr(ref_time_values, function(t) { | ||
| x = purrr::map_dfr(ref_versions, function(t) { |
There was a problem hiding this comment.
Please rename t -> ref_time_value to improve consistency with the current naming approach.
R/archive.R
Outdated
| @@ -701,13 +703,13 @@ epi_archive = | |||
| f = function(x, quo, ...) rlang::eval_tidy(quo, x) | |||
| new_col = sym(names(rlang::quos_auto_name(quos))) | |||
|
|
|||
| x = purrr::map_dfr(ref_time_values, function(t) { | |||
| x = purrr::map_dfr(ref_versions, function(t) { | |||
tests/testthat/test-epix_slide.R
Outdated
| }) | ||
|
|
||
| test_that("epix_slide works as intended",{ | ||
| x2 <- ea$clone()$DT %>% |
There was a problem hiding this comment.
Is it possible to construct x2 from scratch in case we change archive_cases_dv_subset, and to make this more readable? (tribble sometimes helps make this easier. Or tribble + unnest, something like tibble::tribble(~v, ~t, 5, 1:4, 6, 1:5) %>% tidyr::unnest(t))
I had pulled in
epi_slideto try to fix a bug, hence it incorporates changes from #188.