From 18b5c4896a50bc118da3028a5f77f2fe53522fe0 Mon Sep 17 00:00:00 2001 From: Sebastian Funk Date: Wed, 20 Mar 2024 09:14:41 +0100 Subject: [PATCH 1/4] avoid returning NULL --- R/pairwise-comparisons.R | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/R/pairwise-comparisons.R b/R/pairwise-comparisons.R index 88bfeccf1..78fef252d 100644 --- a/R/pairwise-comparisons.R +++ b/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( + "!" = "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( From b8016598a3dfc59884cee63adcc28ea470220d5e Mon Sep 17 00:00:00 2001 From: Sebastian Funk Date: Wed, 20 Mar 2024 09:16:14 +0100 Subject: [PATCH 2/4] add news item --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 4f1e33cf7..7a388ff66 100644 --- a/NEWS.md +++ b/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 From 68d256c96eed15e6833c75e876cf6af5dd70e304 Mon Sep 17 00:00:00 2001 From: Sebastian Funk Date: Wed, 20 Mar 2024 09:27:23 +0100 Subject: [PATCH 3/4] fix `cli_abort` argument --- R/pairwise-comparisons.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/pairwise-comparisons.R b/R/pairwise-comparisons.R index 78fef252d..381345c03 100644 --- a/R/pairwise-comparisons.R +++ b/R/pairwise-comparisons.R @@ -232,7 +232,7 @@ pairwise_comparison_one_group <- function(scores, # if there aren't enough models to do any comparison, abort if (length(models) < 2) { cli_abort( - "!" = "There are not enough models to do any comparison" + c("!" = "There are not enough models to do any comparison") ) } From b48ae3f3e71dfe8982b0a6da76e6c32a217aab63 Mon Sep 17 00:00:00 2001 From: Sebastian Funk Date: Wed, 20 Mar 2024 09:27:34 +0100 Subject: [PATCH 4/4] update tests --- tests/testthat/test-pairwise_comparison.R | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/testthat/test-pairwise_comparison.R b/tests/testthat/test-pairwise_comparison.R index 1892a6fbe..5dffe29b2 100644 --- a/tests/testthat/test-pairwise_comparison.R +++ b/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