908: Flexible model column

[sbfnk]

Description

This PR closes #908. It also closes #914.

It removes all hard coding of the model column, which introduces breaking changes both in summarise_scores() and get_pairwise_comparison() as discussed in #908 (comment)

The change in summarise_scores() could be made unbroken by setting the default for by back to by = "model", perhaps for a transition period.

In get_pairwise_comparison() I realised this change by splitting the existing by argument into by and compare arguments, rather than (as previously) interpreting the first vector element to be special if equal "model" or otherwise not. This means that this can now make pairwise comparisons amongst everything but also that it breaks existing code (which could be mitigated by a default compare = "model" as above).

Checklist

• My PR is based on a package issue and I have explicitly linked it.
• I have included the target issue or issues in the PR title as follows: issue-number: PR title
• I have tested my changes locally.
• I have added or updated unit tests where necessary.
• I have updated the documentation if required.
• I have built the package locally and run rebuilt docs using roxygen2.
• My code follows the established coding standards and I have run lintr::lint_package() to check for style issues introduced by my changes.
• I have added a news item linked to this PR.
• I have reviewed CI checks for this PR and addressed them as far as I am able.

[sbfnk]

R-CMD-check for v4.0 fails for unrelated reasons, reported at mitchelloharawild/distributional#128

[seabbs]

I like compare alot and more generally like this change.

See suggestion for having "model" as a soft requirement and so using it as a default with appropriate warnings.

Does this change mean that all current forecast_units need to be updated to include "model"?

[seabbs]

We also might want to solicit some user feedback here.

[sbfnk]

Does this change mean that all current forecast_units need to be updated to include "model"?

Don't they already?

> get_forecast_unit(score(example_sample_continuous))
[1] "location"        "location_name"   "target_end_date" "target_type"    
[5] "forecast_date"   "model"           "horizon"        

[seabbs]

isn't that because when it was saved model was being treated as a special var and we were inserting it into the forecast unit?

[seabbs]

The answer to my question is no we had no special handling for model in forecast unit and so this will work without changes (

scoringutils/R/convenience-functions.R

Line 276 in 2c363c8

set_forecast_unit <- function(data, forecast_unit) {

)

[nikosbosse]

Nice, thanks a lot!

My intuition is we might want to set "model" as the default for compare (or by, where appropriate) and just have an informative error if you don't have a "model" column available.
That would make things simple for everyone who has a model column and everyone else has to specify the argument anyway.

I don't have a very strong preference though. What do you think?

[seabbs]

I don't have a very strong preference though. What do you think?

+1 for this option

[nikosbosse]

@sbfnk what do you think? Also I'm happy to do the leg-work of updating the PR if you prefer

[sbfnk]

So just to check we're all in agreement we want to have

summarise_scores(scores) ## summarise by model, error if column not present (1)
summarise_scores(scores, by = NULL) ## summarise across everything (2)
summarise_scores(scores, by = "target_type") ## summarise by target type (3)
summarise_scores(scores, by = c("model", "target_type")) ## summarise by model and target type (4)

I'd find a by = NULL default (which will work whatever the columns are) a more intuitive choice but am happy to make the change if agreement is that this is preferred.

[seabbs]

I think 2 as the default is nice if we think more users will just want to summarise a single model than will be annoyed by the breaking change of going from model as default to not.

I don't really have a strong view either way and we can always change this default latter so I don't see it as absolutely crucial to be 💯 on the right choice.

[nikosbosse]

So just to check we're all in agreement we want to have

I think the following should error:

summarise_scores(scores, by = NULL) ## summarise across everything (2)

It currently error as well, but we're not catching it correctly.

In summarise_scores() we'd need to update

assert_subset(by, names(scores), empty.ok = TRUE)

to

assert_subset(by, names(scores), empty.ok = FALSE)

and add e.g. the following test:

test_that("summarise_scores() errors if `by = NULL", {
  expect_error(
    summarise_scores(scores_quantile, by = NULL),
    "Assertion on 'by' failed: Must be a subset of"
  )
})

(EDIT: Just did that in #919)

Other than that I'd suggest to set/keep the following defaults:

summarise_scores <- function(scores, by = "model", fun = mean, ...)
get_pairwise_comparisons <- function(scores,  compare,  by = NULL/character(0))

for get_pairwise_comparisons: we usually use NULL for empty arguments. character(0) has the advantage that it can be directly plugged in split(), whereas with NULL we'd have to handle that. I'm still leaning slightly towards NULL as the default for consistency with other functions. Not a very strong preference though

[nikosbosse]

Nice, this looks great!
I added a suggestion to correct a typo. Other than that only one minor bit:

get_pairwise_comparisons() has currently a default by = NULL
add_relative_skill() has a default by = character(0) - that should probably be changed to by = NULL

[nikosbosse]

Nice!