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

[nikosbosse]

As discussed in #791:

Should we replace customise_metric by purrr::partial?

pro:

• fewer NAMESPACE exports
• less code to maintain
• users already familiar with partial don't have to deal with a new function

con:

• we lose the current documentation and have to find other, arguable less convenient ways to document the workflow
• we rely on an external function and have to adapt to it when it changes
• we lose the chance to add further checks in the future

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

---

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]

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

[seabbs]

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]

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]

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]

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.

[seabbs]

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.

I thought we had agreed on what @Bisaloo is suggesting here? I think this is the way to go.

[nikosbosse]

Ah - in light of this statement of yours

I think of these I am leaning towards keeping as is

That was not clear to me. If you both agree then I'm happy to make the change to purrr::partial

[nikosbosse]

Working on swapping in purrr::partial() for customise_metric(). I do see one (and half) meaningful changes:

The first one is that customise_metric() is storing the object and it gets as part of the function definition, whereas purrr::partial() does not:

  # works with customise_metric
  argument <- c("hi", "hello", "I'm here")
  custom_metric <- customise_metric(print, x = argument)
  expect_output(custom_metric(), "I'm here")

  argument <- NULL
  expect_output(custom_metric(), "I'm here")

  # fails with purrr::partial
  argument <- c("hi", "hello", "I'm here")
  custom_metric <- purrr::partial(print, x = argument)
  expect_output(custom_metric(), "I'm here")

  argument <- NULL
  expect_output(custom_metric(), "I'm here") 
> Error: `custom_metric\(\)` does not match "I'm here".
> Actual value: "NULL"

The second change is that customise_metric() fails immediately instead of at runtime

  # passes with customise_metric
  expect_error(
    custom_metric <- customise_metric(print, x = doesnotexist),
    "object 'doesnotexist' not found"
  )

  # fails to produce an error with purrr::partial
  expect_error(
    custom_metric <- purrr::partial(print, x = doesnotexist),
    "object 'doesnotexist' not found"
  )

What do you think about those?

[Bisaloo]

argument <- c("hi", "hello", "I'm here")
custom_metric <- purrr::partial(print, x = !!argument)
testthat::expect_output(custom_metric(), "I'm here")

argument <- NULL
testthat::expect_output(custom_metric(), "I'm here") 

^{Created on 2024-07-27 with reprex v2.1.1}

?purrr::partial says:

Please unquote the arguments that should be evaluated once at function creation time with ⁠!!⁠.

[nikosbosse]

Reading is an underrated skill... That makes a lot of sense, thank you! I'll add that to the documentation.

[seabbs]

The second issue is still the case right? I think its a bit less friendly but not the end of the world

[nikosbosse]

both are solved by unquoting!

[seabbs]

ah awesome!