Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add coverage deviation as a metric #417

Merged
merged 56 commits into from
Nov 15, 2023

Conversation

nikosbosse
Copy link
Contributor

This PR adds coverage deviation as a new metric. The function is obviously named terribly.
Maybe interval_coveragedeviation_quantile() is better?
At the moment, just coverage_deviation or something like that would also work. But in theory you could also do quantile_coverage_deviation that does the same thing for quantiles instead of intervals...

I guess a question is whether we think it's a useful metric overall? Not great, not terrible?

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (rework-add_coverage()@32239bf). Click here to learn what that means.

❗ Current head b1fba6c differs from pull request most recent head ad411da. Consider uploading reports for the commit ad411da to get more accurate results

Additional details and impacted files
@@                   Coverage Diff                    @@
##             rework-add_coverage()     #417   +/-   ##
========================================================
  Coverage                         ?   78.97%           
========================================================
  Files                            ?       20           
  Lines                            ?     1774           
  Branches                         ?        0           
========================================================
  Hits                             ?     1401           
  Misses                           ?      373           
  Partials                         ?        0           

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

@nikosbosse nikosbosse changed the base branch from main to add-wis-components November 10, 2023 11:37
… and other functions with the same arguments
… other data documentation (seems it was ignored anyway)
…ion of coverage_deviation per quantile as a metric
Copy link
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.

Aside from not being totally happy with where we have landed with one vector as the output for a metric (which leads us to this place) I think this is all fine.

Isn't this interval deviation though? Don't we need both?

@@ -32,6 +32,7 @@ export(crps_sample)
export(dispersion)
export(dss_sample)
export(get_duplicate_forecasts)
export(interval_coverage_deviation_quantile)
Copy link
Contributor

Choose a reason for hiding this comment

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

so the reason we have to do all this stuff is we are only supporting one metric per function call vs allowing the output to be a data.frame and unnesting?

Copy link
Contributor Author

@nikosbosse nikosbosse Nov 15, 2023

Choose a reason for hiding this comment

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

I mean, we could have all functions return a data.frame and then merging things together in score().
Or a wrapper in score() that checks whether the output is a data.frame, list or a vector? I'd be open to that, I just haven't thought about it long enough to have a good vision for how it would look like.
I created an issue, $455. Could you please elaborate a bit on how you'd imagine this?

#' )
#' quantile <- c(0.1, 0.25, 0.5, 0.75, 0.9)
#' interval_coverage_deviation_quantile(observed, predicted, quantile)
interval_coverage_deviation_quantile <- function(observed, predicted, quantile) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this feels like it has a lot of duplication from add_coverage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened an issue #454

Base automatically changed from add-wis-components to rework-add_coverage() November 15, 2023 17:07
@nikosbosse
Copy link
Contributor Author

Yeah that's interval_coverage_deviation. I created an issue #453 as a reminder to create an equivalent function for quantile coverage

@nikosbosse nikosbosse merged commit 8fe6e2e into rework-add_coverage() Nov 15, 2023
@nikosbosse nikosbosse deleted the add-coverage-deviation branch November 15, 2023 21:35
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.

3 participants