epiforecasts / scoringutils

Utilities for Scoring and Assessing Predictions
https://epiforecasts.io/scoringutils/
Other
47 stars 19 forks source link

Decide on whether to use purrr::partial vs. customise_metric() #820

Open nikosbosse opened 1 month ago

nikosbosse commented 1 month ago

As discussed in #791:

Should we replace customise_metric by purrr::partial?

pro:

con:

Rogue alternative: using customise_metric() as an alias that directly calls partial?

image

full function code:

#' Customises a metric function with additional arguments.
#'
#' @description
#' This function takes a metric function and additional arguments, and returns
#' a new function that includes the additional arguments when calling the
#' original metric function.
#'
#' This is the expected way to pass additional
#' arguments to a metric when evaluating a forecast using [score()]:
#' To evaluate a forecast using a metric with an additional argument, you need
#' to create a custom version of the scoring function with the argument
#' included. You then need to create an updated version of the list of scoring
#' functions that includes your customised metric and pass this to [score()].
#'
#' @param metric The metric function to be customised.
#' @param ... Additional arguments to be included when calling the metric
#'   function.
#'
#' @return A customised metric function.
#' @keywords metric
#'
#' @export
#' @examples
#' # Create a customised metric function
#' custom_metric <- customise_metric(mean, na.rm = TRUE)
#'
#' # Use the customised metric function
#' values <- c(1, 2, NA, 4, 5)
#' custom_metric(values)
#'
#' # Updating metrics list to calculate 70% coverage in `score()`
#' interval_coverage_70 <- customise_metric(
#'   interval_coverage, interval_range = 70
#' )
#' updated_metrics <- c(
#'   metrics_quantile(),
#'   "interval_coverage_70" = interval_coverage_70
#' )
#' score(
#'   as_forecast(example_quantile),
#'   metrics = updated_metrics
#' )
customise_metric <- function(metric, ...) {
  assert_function(metric)
  dots <- list(...)
  customised_metric <- function(...) {
    do.call(metric, c(list(...), dots))
  }
  return(customised_metric)
}
nikosbosse commented 1 month ago

Pinging @seabbs @Bisaloo @sbfnk @elray1 @nickreich @jamesmbaazam @jhellewell14 for people who might have opinions

seabbs commented 1 month ago

I don't feel that strongly either way on this but I am not sure its working picking up a dependency just for this?

We could avoid that by just documenting that one could use purrr::partial (and maybe it would need to be a suggests) so that seems like a better way to go vs wrapping it.

One key question here is are we likely to add further checks in the future or other custom functionality?

nikosbosse commented 1 month ago

One key question here is are we likely to add further checks in the future or other custom functionality? Unsure I guess. You had a vision for actually doing proper checking of functions which might play into this.

So current plan is to document and make clear to users that this is basically the same as purrr::partial?

seabbs commented 1 month ago

No that is one option. The other is to leave as is.

It depends on if we like having it as part of the main stream docs and if we imagine future functionality. Do we?

I think of these I am leaning towards keeping as is

Bisaloo commented 1 month ago

My idea (but I may be missing some subtleties) would be to remove customise_metric() and update the docs to always recommend purrr::partial() in its place.