1204: Error when summarise_scores() by contains metric columns

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).

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(

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