suggestions from code review

hubverse-org · Dec 20, 2024 · 285a563 · 285a563
1 parent 82def6f
commit 285a563
Show file tree

Hide file tree

Showing 13 changed files with 38 additions and 24 deletions.
diff --git a/R/config.R b/R/config.R
@@ -8,7 +8,7 @@
 read_config <- function(hub_path, config_path) {
   tryCatch(
     {
-      config <- yaml::read_yaml(config_path)
+      config <- yaml::read_yaml(config_path, eval.expr = FALSE)
     },
     error = function(e) {
       # This handler is used when an unrecoverable error is thrown while

diff --git a/R/load_model_out_in_window.R b/R/load_model_out_in_window.R
@@ -13,7 +13,8 @@ load_model_out_in_window <- function(hub_path, target_id, eval_window) {
 
   # filter to the requested target_id
   hub_tasks_config <- hubUtils::read_config(hub_path, config = "tasks")
-  task_groups <- hub_tasks_config[["rounds"]][[1]][["model_tasks"]]
+  round_ids <- hubUtils::get_round_ids(hub_tasks_config)
+  task_groups <- hubUtils::get_round_model_tasks(hub_tasks_config, round_ids[1])
   target_ids_by_task_group <- get_target_ids_by_task_group(task_groups)
   task_group_idxs_w_target <- get_task_group_idxs_w_target(target_id, target_ids_by_task_group)
 
@@ -28,7 +29,8 @@ load_model_out_in_window <- function(hub_path, target_id, eval_window) {
     dplyr::filter(!!rlang::sym(target_task_id_var_name) == target_task_id_value)
 
   # if eval_window doesn't specify any subsetting by rounds, return the full data
-  if (identical(names(eval_window), "window_name")) {
+  no_limits <- identical(names(eval_window), "window_name")
+  if (no_limits) {
     return(conn |> dplyr::collect())
   }
 
@@ -44,7 +46,6 @@ load_model_out_in_window <- function(hub_path, target_id, eval_window) {
 
   if ("n_last_round_ids" %in% names(eval_window)) {
     # filter to the last n rounds
-    round_ids <- hubUtils::get_round_ids(hub_tasks_config)
     max_present_round_id <- max(model_out_tbl[[round_id_var_name]])
     round_ids <- round_ids[round_ids <= max_present_round_id]
     round_ids <- utils::tail(round_ids, eval_window$n_last_round_ids)

diff --git a/R/hub_tasks_config_utils.R → R/utils-hub_tasks_config.R b/R/hub_tasks_config_utils.R → R/utils-hub_tasks_config.R
@@ -1,4 +1,8 @@
 #' For each task group, get the target_id entries from its target_metadata
+#'
+#' @return a list with one entry for each task group,
+#' where each entry is a character vector of target_ids for that group
+#'
 #' @noRd
 get_target_ids_by_task_group <- function(task_groups) {
   result <- purrr::map(
@@ -15,7 +19,8 @@ get_target_ids_by_task_group <- function(task_groups) {
 }
 
 
-#' Get the indices of elements of target_ids_by_task_group that contain the target_id
+#' Get an integer vector with the indices of elements of
+#' target_ids_by_task_group that contain the target_id
 #' @noRd
 get_task_group_idxs_w_target <- function(target_id, target_ids_by_task_group) {
   result <- purrr::map2(

diff --git a/tests/testthat/_snaps/config.md b/tests/testthat/_snaps/config.md
@@ -2,8 +2,6 @@
 
     Code
       read_config(hub_path, test_path("testdata", "test_configs", "config_valid.yaml"))
-    Message
-      i Updating superseded URL `Infectious-Disease-Modeling-hubs` to `hubverse-org`
     Output
       $targets
       $targets[[1]]
@@ -235,8 +233,6 @@
     Code
       read_config(hub_path, test_path("testdata", "test_configs",
         "config_valid_no_min_round_id.yaml"))
-    Message
-      i Updating superseded URL `Infectious-Disease-Modeling-hubs` to `hubverse-org`
     Output
       $targets
       $targets[[1]]
@@ -454,8 +450,6 @@
     Code
       read_config(hub_path, test_path("testdata", "test_configs",
         "config_valid_no_disaggregate_by.yaml"))
-    Message
-      i Updating superseded URL `Infectious-Disease-Modeling-hubs` to `hubverse-org`
     Output
       $targets
       $targets[[1]]
@@ -681,8 +675,6 @@
     Code
       read_config(hub_path, test_path("testdata", "test_configs",
         "config_valid_no_task_id_text.yaml"))
-    Message
-      i Updating superseded URL `Infectious-Disease-Modeling-hubs` to `hubverse-org`
     Output
       $targets
       $targets[[1]]

diff --git a/tests/testthat/helper-expect_df_equal_up_to_order.R b/tests/testthat/helper-expect_df_equal_up_to_order.R
@@ -5,9 +5,9 @@
 expect_df_equal_up_to_order <- function(df_act, df_exp) {
   cols <- colnames(df_act)
   testthat::expect_equal(cols, colnames(df_exp))
-  testthat::expect_true(isTRUE(all.equal(
+  testthat::expect_equal(
     dplyr::arrange(df_act, dplyr::across(dplyr::all_of(cols))),
     dplyr::arrange(df_exp, dplyr::across(dplyr::all_of(cols))),
-    check.attributes = FALSE
-  )))
+    ignore_attr = FALSE
+  )
 }
diff --git a/tests/testthat/test-load_model_out_in_window.R b/tests/testthat/test-load_model_out_in_window.R
@@ -80,6 +80,10 @@ test_that(
       model_out_tbl,
       expected_model_out_tbl
     )
+    expect_setequal(
+      unique(model_out_tbl$reference_date),
+      c("2022-12-17", "2023-01-14")
+    )
 
     # n_last_round_ids = 4: we expect 2022-12-24, 2022-12-31, 2023-01-07, 2023-01-14
     # (but note that ecfh has only 2023-01-14 from this set)
@@ -105,6 +109,10 @@ test_that(
       model_out_tbl,
       expected_model_out_tbl
     )
+    expect_setequal(
+      unique(model_out_tbl$reference_date),
+      c("2023-01-14")
+    )
   }
 )
 
@@ -135,6 +143,10 @@ test_that(
       model_out_tbl,
       expected_model_out_tbl
     )
+    expect_setequal(
+      unique(model_out_tbl$reference_date),
+      c("2023-01-14")
+    )
   }
 )
 
@@ -147,7 +159,7 @@ test_that(
       target_id = "wk flu hosp rate category",
       eval_window = list(
         window_name = "some subset",
-        min_round_id = "2022-11-19",
+        min_round_id = "2022-11-19", # there are 9 rounds on or after this date
         n_last_round_ids = 5
       )
     )
@@ -165,5 +177,9 @@ test_that(
       model_out_tbl,
       expected_model_out_tbl
     )
+    expect_setequal(
+      unique(model_out_tbl$reference_date),
+      c("2022-12-17", "2023-01-14")
+    )
   }
 )
diff --git a/tests/testthat/testdata/create_ecfh.R b/tests/testthat/testdata/create_ecfh.R
@@ -14,7 +14,7 @@
 library(dplyr)
 library(hubData)
 
-hub_path <- "tests/testthat/testdata/ecfh"
+hub_path <- testthat::test_path("testdata", "ecfh")
 
 models <- list.dirs(file.path(hub_path, "model-output"),
                     full.names = FALSE, recursive = FALSE)

diff --git a/tests/testthat/testdata/ecfh/hub-config/admin.json b/tests/testthat/testdata/ecfh/hub-config/admin.json
@@ -1,5 +1,5 @@
 {
-    "schema_version": "https://raw.githubusercontent.com/Infectious-Disease-Modeling-Hubs/schemas/main/v3.0.0/admin-schema.json",
+    "schema_version": "https://raw.githubusercontent.com/hubverse-org/schemas/main/v3.0.0/admin-schema.json",
     "name": "Example complex forecast hub",
     "maintainer": "hubverse",
     "contact": {

diff --git a/tests/testthat/testdata/ecfh/hub-config/tasks.json b/tests/testthat/testdata/ecfh/hub-config/tasks.json
@@ -1,5 +1,5 @@
 {
-    "schema_version": "https://raw.githubusercontent.com/Infectious-Disease-Modeling-Hubs/schemas/main/v3.0.0/tasks-schema.json",
+    "schema_version": "https://raw.githubusercontent.com/hubverse-org/schemas/main/v3.0.0/tasks-schema.json",
     "rounds": [
         {
             "round_id_from_variable": true,

diff --git a/tests/testthat/testdata/test_hub_invalid_mult_rnd/hub-config/admin.json b/tests/testthat/testdata/test_hub_invalid_mult_rnd/hub-config/admin.json
@@ -1,5 +1,5 @@
 {
-    "schema_version": "https://raw.githubusercontent.com/Infectious-Disease-Modeling-Hubs/schemas/main/v3.0.0/admin-schema.json",
+    "schema_version": "https://raw.githubusercontent.com/hubverse-org/schemas/main/v3.0.0/admin-schema.json",
     "name": "Example complex forecast hub",
     "maintainer": "hubverse",
     "contact": {

diff --git a/tests/testthat/testdata/test_hub_invalid_mult_rnd/hub-config/tasks.json b/tests/testthat/testdata/test_hub_invalid_mult_rnd/hub-config/tasks.json
@@ -1,5 +1,5 @@
 {
-    "schema_version": "https://raw.githubusercontent.com/Infectious-Disease-Modeling-Hubs/schemas/main/v3.0.0/tasks-schema.json",
+    "schema_version": "https://raw.githubusercontent.com/hubverse-org/schemas/main/v3.0.0/tasks-schema.json",
     "rounds": [
         {
             "round_id_from_variable": true,

diff --git a/tests/testthat/testdata/test_hub_invalid_rifv_F/hub-config/admin.json b/tests/testthat/testdata/test_hub_invalid_rifv_F/hub-config/admin.json
@@ -1,5 +1,5 @@
 {
-    "schema_version": "https://raw.githubusercontent.com/Infectious-Disease-Modeling-Hubs/schemas/main/v3.0.0/admin-schema.json",
+    "schema_version": "https://raw.githubusercontent.com/hubverse-org/schemas/main/v3.0.0/admin-schema.json",
     "name": "Example complex forecast hub",
     "maintainer": "hubverse",
     "contact": {

diff --git a/tests/testthat/testdata/test_hub_invalid_rifv_F/hub-config/tasks.json b/tests/testthat/testdata/test_hub_invalid_rifv_F/hub-config/tasks.json
@@ -1,5 +1,5 @@
 {
-    "schema_version": "https://raw.githubusercontent.com/Infectious-Disease-Modeling-Hubs/schemas/main/v3.0.0/tasks-schema.json",
+    "schema_version": "https://raw.githubusercontent.com/hubverse-org/schemas/main/v3.0.0/tasks-schema.json",
     "rounds": [
         {
             "round_id_from_variable": false,