First step towards reworking score.scoringutils_quantile() - adding coverage as a metric
#395
Conversation
Update branch to include coverage functions
There was a problem hiding this comment.
This looks fine but as discussed on our call last week I don't like the suggested usage of run_safely and tbh am not entirely convinced of the idea of arg passing as you are proposing for score.
Note that this PR is a good example where there is next to no information about what has changed/why it has changed in a way that makes it easy to understand the decision underlying the code changes.
| @@ -92,7 +92,7 @@ interval_coverage_quantile <- function(observed, predicted, quantile, range = 50 | |||
| assert_number(range) | |||
| necessary_quantiles <- c((100 - range) / 2, 100 - (100 - range) / 2) / 100 | |||
| if (!all(necessary_quantiles %in% quantile)) { | |||
| rlang::warn( | |||
| warning( | |||
There was a problem hiding this comment.
why are you changing this back?
There was a problem hiding this comment.
It errored 🥲 So I made an issue (#415) and switched it back for now. In hindsight I see your point about adding that as context here... Apologies!
| lapply(seq_along(metrics), function(i, ...) { | ||
| metric_name <- names(metrics[i]) | ||
| fun <- metrics[[i]] | ||
| matching_args <- filter_function_args(fun, args) |
There was a problem hiding this comment.
I thought we agreed that run_safely would be used here?
There was a problem hiding this comment.
or that we wouldn't do this at all
| #' @param ... Arguments to pass to `fun` | ||
| #' @param fun A function to execute | ||
| #' @return The result of `fun` or `NULL` if `fun` errors | ||
| #' @export |
There was a problem hiding this comment.
I think we want this to be used internally not in the metrics list
|
|
||
| if (inherits(result, "try-error")) { | ||
| msg <- conditionMessage(attr(result, "condition")) | ||
| warning( |
There was a problem hiding this comment.
rlang or base what are you using?
This PR is a first step towards reworking
score.scoringutils_quantile(). The work isn't done, but I figured it would be better to keep PR size reasonable.. :)This PR
score.scoringutils_quantile()(currently calledscore.scoringutils_quantile_new()run_safely()which allows to run functions safely regardless of the arguments passed to itmetrics_quantile, the default metrics forscore.scoringutils_quantile()to includeinterval_coveragefor interval levels 50 and 90.Left to do:
score.scoringutils_quantile()wis()withinscore.scoringutils_quantile()