Skip to content

Issue #436 Rename instances of "coverage" to either "interval coverage" or "quantile coverage"#540

Merged
seabbs merged 4 commits intodevelopfrom
rename-coverage
Jan 4, 2024
Merged

Issue #436 Rename instances of "coverage" to either "interval coverage" or "quantile coverage"#540
seabbs merged 4 commits intodevelopfrom
rename-coverage

Conversation

@nikosbosse
Copy link
Copy Markdown
Collaborator

@nikosbosse nikosbosse commented Jan 3, 2024

Description

This PR closes #436.

We previously used the word "coverage" to describe interval coverage. This sensationally boring PR

  • replaces instances of "coverage" both in the code and in the documentation to make this distinction a bit clearer.
  • updates the default scoring rules
  • makes a few changes to plotting functions (plot_score_table()) and to available_metrics(). available_metrics() needs to be removed at some point in the future and plot_score_table() also needs an update, but this should be done in a different PR

Mentioning a few unrelated issues that came up:

  • There are some suggestions for code refactoring in interval_coverage_sample(). These should be moved to an issue. Alternatively, (and maybe preferably?) the function should be removed altogether. We're not using it in score() (i.e. it is not part of the default functions for sample-based forecasts). For data.frame-based forecasts, our preferred workflow is to transform forecasts to a quantile format explicitly and then e.g. use add_coverage(). We don't have any equivalent workflow for functions in a matrix-based format. But then again maybe we don't need to. We could still think about a sample_to_quantile.numeric() function which would transform a matrix of samples to a data.frame in a quantile-based format.
  • We'll have to rethink plot_score_table(). The function currently uses different colour scales depending on whether a score is "low is better" or "zero is better" or something else. Since we now allow users to supply their own functions that won't work anymore.

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.

@nikosbosse nikosbosse changed the title Issue # Rename instances of "coverage" to either "interval coverage" or "quantile coverage" Issue #436 Rename instances of "coverage" to either "interval coverage" or "quantile coverage" Jan 3, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 3, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (5c7bdb9) 83.69% compared to head (47207e8) 83.71%.

Files Patch % Lines
R/metrics-quantile.R 72.72% 3 Missing ⚠️
R/metrics-sample.R 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #540      +/-   ##
===========================================
+ Coverage    83.69%   83.71%   +0.01%     
===========================================
  Files           21       21              
  Lines         1717     1719       +2     
===========================================
+ Hits          1437     1439       +2     
  Misses         280      280              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nikosbosse nikosbosse marked this pull request as ready for review January 3, 2024 21:16
@nikosbosse nikosbosse requested a review from seabbs January 3, 2024 21:16
@seabbs
Copy link
Copy Markdown
Contributor

seabbs commented Jan 4, 2024

We'll have to rethink plot_score_table(). The function currently uses different colour scales depending on whether a score is "low is better" or "zero is better" or something else. Since we now allow users to supply their own functions that won't work anymore.

Yeah, this is the cost of making things more general I guess. I wonder if we should remove the colouring in the first instance?

There are some suggestions for code refactoring in interval_coverage_sample(). These should be moved to an issue. Alternatively, (and maybe preferably?) the function should be removed altogether. We're not using it in score() (i.e. it is not part of the default functions for sample-based forecasts). For data.frame-based forecasts, our preferred workflow is to transform forecasts to a quantile format explicitly and then e.g. use add_coverage(). We don't have any equivalent workflow for functions in a matrix-based format. But then again maybe we don't need to. We could still think about a sample_to_quantile.numeric() function which would transform a matrix of samples to a data.frame in a quantile-based format.

I'm a bit lost. Can we move to an issue please!

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.

LGTM

@seabbs seabbs merged commit 2ff165f into develop Jan 4, 2024
@seabbs seabbs deleted the rename-coverage branch January 4, 2024 22:41
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