Skip to content

Rework score.scoringutils quantile()2#421

Merged
nikosbosse merged 13 commits intoadd-coverage-deviationfrom
rework-score.scoringutils_quantile()2
Nov 15, 2023
Merged

Rework score.scoringutils quantile()2#421
nikosbosse merged 13 commits intoadd-coverage-deviationfrom
rework-score.scoringutils_quantile()2

Conversation

@nikosbosse
Copy link
Copy Markdown
Collaborator

This PR

  • fixes a few issues with ae_median_...() that were forgotten in the last PR
  • Corrects a small issue with pit() where pit() relied on the existence of quantile coverage for quantile-based forecasts
  • replaces the old with the new score.scoringutils_quantile()
    • makes an improvement to the new score.scoringutils_quantile(), where the way that the data is split internally is improved to make sure that all forecasts in a single group have the same quantile-levels (vs. previously just the same number of unique quantile levels)
    • changes the metric name interval_score to wis everywhere
    • updates tests where necessary
    • comments out tests that are currently failing since a lot of functions depended on the previous way that scores were returned (i.e. one score per quantile)

@nikosbosse nikosbosse requested a review from seabbs November 13, 2023 16:07
Copy link
Copy Markdown
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes a few issues wit

Like what? This is the kind of thing the PR info is for or even better yet issues.

comments out tests that are currently failing since a lot of functions depended on the previous way that scores were returned (i.e. one score per quantile)

Make an issue for this.

It seems weird that the default metrics are being stored as data when they are just function? If you made it a function you could also make it easier for people to compose lists of related metrics (i.e to get multiple coverage metrics or all the wis types without hating their life).


available_metrics <- function() {
return(unique(scoringutils::metrics$Name))
return(unique(c(scoringutils::metrics$Name,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the right place to break this line is after return( and then unique( and so on vs randomly at the end.

"coverage_50" = \(...) {run_safely(..., range = 50, fun = interval_coverage_quantile)},
"coverage_90" = \(...) {run_safely(..., range = 90, fun = interval_coverage_quantile)},
"coverage_deviation" = interval_coverage_deviation_quantile
"coverage_deviation" = interval_coverage_deviation_quantile,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we storing these as data when they are really a list of functions? Why isn't it just a function?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I completely understand what you're suggesting. Made an issue: #449

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of storing as a data object have this be the return value of some function.

Base automatically changed from rework_ae_median to add-coverage-deviation November 15, 2023 15:03
@nikosbosse nikosbosse merged commit c6c2dfc into add-coverage-deviation Nov 15, 2023
@nikosbosse nikosbosse deleted the rework-score.scoringutils_quantile()2 branch November 15, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants