Merge pull request #717 from epiforecasts/pairwise-error

637: Error in pairwise comparison

NEWS.md

@@ -53,6 +53,7 @@ The update introduces breaking changes. If you want to keep using the older vers
 - Deleted the function `plot_ranges()`. If you want to continue using the functionality, you can find the function code [here](https://github.com/epiforecasts/scoringutils/issues/462) or in the visualisation Vignette.
 - Removed the function `plot_predictions()`, as well as its helper function `make_NA()`, in favour of a dedicated Vignette that shows different ways of visualising predictions. For future reference, the function code can be found [here](https://github.com/epiforecasts/scoringutils/issues/659) (Issue #659).
 - Added a first versino of a dedicated Vignette that displays some possible ways of visualising forecasts. 
+- Replaced warnings with errors in `pairwise_comparison` to avoid returning `NULL`
 
 # scoringutils 1.2.2
 

R/pairwise-comparisons.R

@@ -119,13 +119,12 @@ pairwise_comparison <- function(
     scores <- scores[!is.na(scores[[metric]])]
     if (nrow(scores) == 0) {
       #nolint start: keyword_quote_linter object_usage_linter
-      cli_warn(
+      cli_abort(
         c(
           "!" = "After removing {.val NA} values for {.var {metric}},
          no values were left."
         )
       )
-      return(NULL)
     }
     cli_warn(
       c(
@@ -227,16 +226,14 @@ pairwise_comparison_one_group <- function(scores,
     )
   }
 
-  if (nrow(scores) == 0) {
-    return(NULL)
-  }
-
   # get list of models
   models <- unique(scores$model)
 
-  # if there aren't enough models to do any comparison, return NULL
+  # if there aren't enough models to do any comparison, abort
   if (length(models) < 2) {
-    return(NULL)
+    cli_abort(
+      c("!" = "There are not enough models to do any comparison")
+    )
   }
 
   # create a data.frame with results
@@ -537,19 +534,17 @@ add_pairwise_comparison <- function(
   # store original metrics
   metrics <- get_metrics(scores)
 
-  if (!is.null(pairwise)) {
-    # delete unnecessary columns
-    pairwise[, c(
-      "compare_against", "mean_scores_ratio",
-      "pval", "adj_pval"
-    ) := NULL]
-    pairwise <- unique(pairwise)
-
-    # merge back
-    scores <- merge(
-      scores, pairwise, all.x = TRUE, by = get_forecast_unit(pairwise)
-    )
-  }
+  # delete unnecessary columns
+  pairwise[, c(
+    "compare_against", "mean_scores_ratio",
+    "pval", "adj_pval"
+  ) := NULL]
+  pairwise <- unique(pairwise)
+
+  # merge back
+  scores <- merge(
+    scores, pairwise, all.x = TRUE, by = get_forecast_unit(pairwise)
+  )
 
   # Update score names
   new_metrics <- paste(

tests/testthat/test-pairwise_comparison.R

@@ -359,7 +359,7 @@ test_that("Basic input checks for `add_pairwise_comparison() work", {
 
   # warning if there are no values left after removing NAs
   eval_nas[, "crps" := NA]
-  expect_warning(
+  expect_error(
     add_pairwise_comparison(
       eval_nas, by = "model", metric = "crps"
     ),
@@ -415,18 +415,18 @@ test_that("pairwise_comparison_one_group() throws error with wrong inputs", {
     "pairwise comparisons require a column called 'model'"
   )
 
-  # expect `NULL` as a result if scores has zero rows
+  # expect error as a result if scores has zero rows
   test <- data.table::copy(scores_continuous)[model == "impossible"]
-  expect_equal(
+  expect_error(
     pairwise_comparison_one_group(test, by = "model", metric = "crps"),
-    NULL
+    "not enough models"
   )
 
-  # expect NULL if there aren't enough models
+  # expect error if there aren't enough models
   test <- data.table::copy(scores_continuous)[model == "EuroCOVIDhub-ensemble"]
-  expect_equal(
+  expect_error(
     pairwise_comparison_one_group(test, by = "model", metric = "crps"),
-    NULL
+    "not enough models"
   )
 
   # expect error if baseline model is missing