refactor: changes from review

dshemetov · dshemetov · commit f45f30effa6e · 2024-04-29T21:22:11.000-07:00
* remove is_epi_archive and delete in epix_slide
* simplify group_by_drop_default
* filter and move library calls to testthat.R
diff --git a/NAMESPACE b/NAMESPACE
@@ -64,7 +64,6 @@ export(filter)
 export(group_by)
 export(group_modify)
 export(growth_rate)
-export(is_epi_archive)
 export(is_epi_df)
 export(is_grouped_epi_archive)
 export(key_colnames)
@@ -94,6 +93,7 @@ importFrom(checkmate,checkInt)
 importFrom(checkmate,check_atomic)
 importFrom(checkmate,check_data_frame)
 importFrom(checkmate,check_names)
+importFrom(checkmate,expect_class)
 importFrom(checkmate,test_set_equal)
 importFrom(checkmate,test_subset)
 importFrom(checkmate,vname)
diff --git a/R/archive.R b/R/archive.R
@@ -652,31 +652,6 @@ group_by.epi_archive <- function(.data, ..., .add = FALSE, .drop = dplyr::group_
 }
 
 
-#' Test for `epi_archive` format
-#'
-#' @param x An object.
-#' @param grouped_okay Optional; Boolean; should a `grouped_epi_archive` also
-#'   count? Default is `FALSE`.
-#' @return `TRUE` if the object inherits from `epi_archive`.
-#'
-#' @export
-#' @examples
-#' is_epi_archive(jhu_csse_daily_subset) # FALSE (this is an epi_df, not epi_archive)
-#' is_epi_archive(archive_cases_dv_subset) # TRUE
-#'
-#' # By default, grouped_epi_archives don't count as epi_archives, as they may
-#' # support a different set of operations from regular `epi_archives`. This
-#' # behavior can be controlled by `grouped_okay`.
-#' grouped_archive <- archive_cases_dv_subset %>% group_by(geo_value)
-#' is_epi_archive(grouped_archive) # FALSE
-#' is_epi_archive(grouped_archive, grouped_okay = TRUE) # TRUE
-#'
-#' @seealso [`is_grouped_epi_archive`]
-is_epi_archive <- function(x, grouped_okay = FALSE) {
-  inherits(x, "epi_archive") || grouped_okay && inherits(x, "grouped_epi_archive")
-}
-
-
 #' Clone an `epi_archive` object.
 #'
 #' @param x An `epi_archive` object.
diff --git a/R/epiprocess.R b/R/epiprocess.R
@@ -7,7 +7,7 @@
 #' @importFrom checkmate assert assert_scalar assert_data_frame anyMissing
 #'             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
+#'             anyInfinite test_subset test_set_equal checkInt expect_class
 #' @importFrom cli cli_abort cli_warn
 #' @importFrom rlang %||%
 #' @name epiprocess
diff --git a/R/grouped_epi_archive.R b/R/grouped_epi_archive.R
@@ -157,8 +157,7 @@ group_by.grouped_epi_archive <- function(
 #'
 #' @export
 group_by_drop_default.grouped_epi_archive <- function(.tbl) {
-  x <- .tbl
-  x$private$drop
+  .tbl$private$drop
 }
 
 
diff --git a/R/methods-epi_archive.R b/R/methods-epi_archive.R
@@ -797,9 +797,6 @@ epix_slide <- function(
     as_list_col = FALSE,
     names_sep = "_",
     all_versions = FALSE) {
-  if (!is_epi_archive(x, grouped_okay = TRUE)) {
-    cli_abort("`x` must be of class `epi_archive` or `grouped_epi_archive`.")
-  }
   UseMethod("epix_slide")
 }
 
diff --git a/man/is_epi_archive.Rd b/man/is_epi_archive.Rd
diff --git a/tests/testthat.R b/tests/testthat.R
@@ -1,4 +1,6 @@
 library(testthat)
 library(epiprocess)
 
+library(dplyr)
+
 test_check("epiprocess")
diff --git a/tests/testthat/test-archive.R b/tests/testthat/test-archive.R
@@ -1,5 +1,3 @@
-library(dplyr)
-
 test_that("first input must be a data.frame", {
   expect_error(as_epi_archive(c(1, 2, 3), compactify = FALSE),
     regexp = "Must be of type 'data.frame'."
diff --git a/tests/testthat/test-autoplot.R b/tests/testthat/test-autoplot.R
@@ -1,7 +1,4 @@
-library(dplyr)
-
 d <- as.Date("2020-01-01")
-
 raw_df_chr <- dplyr::bind_rows(
   dplyr::tibble(geo_value = "ak", time_value = d + 1:5, value = "a"),
   dplyr::tibble(geo_value = "al", time_value = d + 1:5, value = "d")
diff --git a/tests/testthat/test-compactify.R b/tests/testthat/test-compactify.R
@@ -1,7 +1,3 @@
-library(epiprocess)
-library(data.table)
-library(dplyr)
-
 dt <- archive_cases_dv_subset$DT
 dt <- filter(dt, geo_value == "ca") %>%
   filter(version <= "2020-06-15") %>%
diff --git a/tests/testthat/test-correlation.R b/tests/testthat/test-correlation.R
@@ -1,5 +1,3 @@
-library(tibble)
-
 test_that("epi_cor throws an error for a non-epi_df for its first argument", {
   expect_error(epi_cor(1:10, 1, 1))
   expect_error(epi_cor(data.frame(x = 1:10), 1, 1))
diff --git a/tests/testthat/test-data.R b/tests/testthat/test-data.R
@@ -1,5 +1,5 @@
 test_that("`archive_cases_dv_subset` is formed successfully", {
-  expect_true(is_epi_archive(archive_cases_dv_subset))
+  expect_class(archive_cases_dv_subset, "epi_archive")
 })
 
 test_that("`delayed_assign_with_unregister_awareness` works as expected on good promises", {
diff --git a/tests/testthat/test-epix_slide.R b/tests/testthat/test-epix_slide.R
@@ -1,5 +1,3 @@
-library(dplyr)
-
 test_that("epix_slide only works on an epi_archive", {
   expect_error(epix_slide(data.frame(x = 1)))
 })
@@ -506,7 +504,7 @@ test_that("epix_as_of and epix_slide with long enough window are compatible", {
 
 test_that("epix_slide `f` is passed an ungrouped `epi_archive` when `all_versions=TRUE`", {
   slide_fn <- function(x, gk, rtv) {
-    expect_true(is_epi_archive(x))
+    expect_class(x, "epi_archive")
     return(NA)
   }
 
diff --git a/tests/testthat/test-methods-epi_archive.R b/tests/testthat/test-methods-epi_archive.R
@@ -1,7 +1,4 @@
-library(dplyr)
-
 ea <- archive_cases_dv_subset
-
 ea2_data <- tibble::tribble(
   ~geo_value, ~time_value, ~version, ~cases,
   "ca", "2020-06-01", "2020-06-01", 1,
@@ -104,7 +101,7 @@ test_that("epix_truncate_version_after returns the same grouping type as input e
 
   ea_as_of <- ea2 %>%
     epix_truncate_versions_after(max_version = as.Date("2020-06-04"))
-  expect_true(is_epi_archive(ea_as_of, grouped_okay = FALSE))
+  expect_class(ea_as_of, "epi_archive")
 
   ea2_grouped <- ea2 %>% group_by(geo_value)
 
diff --git a/vignettes/archive.Rmd b/vignettes/archive.Rmd
@@ -113,15 +113,6 @@ key(x$DT)
 In general, the last version of each observation is carried forward (LOCF) to
 fill in data between recorded versions.
 
-```{r}
-original_value <- x$DT$percent_cli[1]
-y <- x # This DOES NOT make a copy of x
-y$DT$percent_cli[1] <- 0
-head(y$DT)
-head(x$DT)
-x$DT$percent_cli[1] <- original_value
-```
-
 ## Some details on metadata
 
 The following pieces of metadata are included as fields in an `epi_archive`

Original file line number	Diff line number	Diff line change
`@@ -157,8 +157,7 @@ group_by.grouped_epi_archive <- function(`
`157`	`157`	`#'`
`158`	`158`	`#' @export`
`159`	`159`	`group_by_drop_default.grouped_epi_archive <- function(.tbl) {`
`160`		`- x <- .tbl`
`161`		`- x$private$drop`
	`160`	`+ .tbl$private$drop`
`162`	`161`	`}`
`163`	`162`
`164`	`163`
Original file line number	Diff line number	Diff line change
`@@ -797,9 +797,6 @@ epix_slide <- function(`
`797`	`797`	`as_list_col = FALSE,`
`798`	`798`	`names_sep = "_",`
`799`	`799`	`all_versions = FALSE) {`
`800`		`- if (!is_epi_archive(x, grouped_okay = TRUE)) {`
`801`		- cli_abort("`x` must be of class `epi_archive` or `grouped_epi_archive`.")
`802`		`- }`
`803`	`800`	`UseMethod("epix_slide")`
`804`	`801`	`}`
`805`	`802`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,3 @@`
`1`		`-library(dplyr)`
`2`		`-`
`3`	`1`	`test_that("first input must be a data.frame", {`
`4`	`2`	`expect_error(as_epi_archive(c(1, 2, 3), compactify = FALSE),`
`5`	`3`	`regexp = "Must be of type 'data.frame'."`