diff --git a/NEWS.md b/NEWS.md index e1ee239eb..f801c6c04 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # scoringutils (development version) +- `summarise_scores()` now errors when `by` contains a metric column (e.g. `by = c("model", "wis")`). Previously, such calls silently returned an unsummarised table with duplicate column names, because the score column was used both as a grouping column and as a column to summarise. This complements the fix for the empty-metrics case in #1179 (#1204). - Added `filter_scores()` and `impute_missing_scores()` for handling missing forecasts before summarisation. `filter_scores()` removes target combinations with insufficient model coverage, while `impute_missing_scores()` fills in missing scores using configurable strategies (worst, mean, NA, or reference model). Both use a strategy function pattern for extensibility. See `vignette("handling-missing-forecasts")` for details (#1122). - Added `plot_discrimination()` to visualise the discrimination ability of binary forecasts by plotting the distribution of predicted probabilities, stratified by the observed outcome. The function requires a `forecast_binary` object (created with `as_forecast_binary()`) (#942). - Fixed `summarise_scores()` producing a data.table with duplicate column names when the input `scores` object had no score columns (e.g. because every metric in `score()` warned and returned nothing). `summarise_scores()` now matches metric columns by exact name rather than regex partial match, and errors with a clear message when there is nothing to summarise (#1179). diff --git a/R/summarise_scores.R b/R/summarise_scores.R index acf53e9f9..e230970e0 100644 --- a/R/summarise_scores.R +++ b/R/summarise_scores.R @@ -15,6 +15,7 @@ #' scores and an additional attribute `metrics` as produced by [score()]). #' @param by Character vector with column names to summarise scores by. Default #' is "model", i.e. scores are summarised by the "model" column. +#' `by` must not contain any of the score (metric) columns themselves. #' @param fun A function used for summarising scores. Default is [mean()]. #' @param ... Additional parameters that can be passed to the summary function #' provided to `fun`. For more information see the documentation of the @@ -60,6 +61,18 @@ summarise_scores <- function(scores, assert_function(fun) metrics <- get_metrics.scores(scores, error = TRUE) + by_metrics <- intersect(by, metrics) + if (length(by_metrics) > 0) { + cli_abort( + c( + `!` = "Cannot summarise scores by a metric column: + {.val {by_metrics}}.", + i = "{.arg by} must only contain columns that identify groups of + forecasts, not the score columns themselves. Remove + {.val {by_metrics}} from {.arg by}." + ) + ) + } metric_cols <- intersect(colnames(scores), metrics) if (length(metric_cols) == 0) { cli_abort( diff --git a/man/summarise_scores.Rd b/man/summarise_scores.Rd index 906955cd5..7065beacd 100644 --- a/man/summarise_scores.Rd +++ b/man/summarise_scores.Rd @@ -14,7 +14,8 @@ summarize_scores(scores, by = "model", fun = mean, ...) scores and an additional attribute \code{metrics} as produced by \code{\link[=score]{score()}}).} \item{by}{Character vector with column names to summarise scores by. Default -is "model", i.e. scores are summarised by the "model" column.} +is "model", i.e. scores are summarised by the "model" column. +\code{by} must not contain any of the score (metric) columns themselves.} \item{fun}{A function used for summarising scores. Default is \code{\link[=mean]{mean()}}.} diff --git a/tests/testthat/test-summarise_scores.R b/tests/testthat/test-summarise_scores.R index b7088b9c3..891de3e32 100644 --- a/tests/testthat/test-summarise_scores.R +++ b/tests/testthat/test-summarise_scores.R @@ -40,6 +40,26 @@ test_that("summarise_scores() handles wrong by argument well", { ) }) +test_that("summarise_scores() errors when `by` contains metric columns", { + # grouping by a score column would silently return an unsummarised table + # with duplicate column names (gh #1204) + expect_error( + summarise_scores(scores_quantile, by = c("model", "wis")), + "metric" + ) + + # the error message should name all offending columns + expect_error( + summarise_scores(scores_quantile, by = c("model", "wis", "bias")), + "\"wis\" and \"bias\"" + ) + + # legitimate calls still work + expect_no_condition( + summarise_scores(scores_quantile, by = get_forecast_unit(scores_quantile)) + ) +}) + test_that("summarise_scores() handles the `metrics` attribute correctly", { test <- data.table::copy(scores_quantile) attr(test, "metrics") <- NULL