epiforecasts / scoringutils

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

Create separate `new_forcast<type>` functions? #856

Closed nikosbosse closed 3 months ago

nikosbosse commented 3 months ago

We currently have a single new_forecast function which creates a new forecast object of the appropriate class via an argument classname which accepts a string.

Alternatively we could have separate new_forcast<type> for the separate forecast types

Current implementation is:

new_forecast <- function(data, classname) {
  data <- as.data.table(data)
  data <- ensure_model_column(data)
  class(data) <- c("forecast", classname, class(data))
  data <- copy(data)
  return(data[])
}

new implementation would probably be something like

new_forecast_sample <- function(data) {
  data <- as.data.table(data)
  data <- ensure_model_column(data)
  class(data) <- c("forecast", "sample", class(data))
  data <- copy(data)
  return(data[])
}
nikosbosse commented 3 months ago

@seabbs you're in favour of this, right? @Bisaloo @jamesmbaazam @sbfnk do you have opinions?

jamesmbaazam commented 3 months ago

The proposed approach will produce a lot of duplicated code. I prefer the existing approach.

seabbs commented 3 months ago

I think the upside is it makes it more obvious to users how to write their own new method and it makes it look more "standard"/ As @jamesmbaazam says though it would be a lot of replicated code if a internal function wasn't used.

Bisaloo commented 3 months ago

The benefit of the first solution is that you can (cleanly) create a forecast of a given type programmatically.

type <- "sample" # actually passed by the user or determined by a more complex code than this
new_forecast(data, type)

The second solution requires jumping through convoluted hoops:

type <- "sample"
do.call(paste0("new_forecast_", type), list(data))

So I would say it mostly depends on how frequently you expect this situation.

nikosbosse commented 3 months ago

I vote for the existing approach as well. Fine to close for now and potentially revisit in the future?

nikosbosse commented 3 months ago

Please feel free to reopen!