Closed seabbs closed 4 months ago
Nice! I like this.
I suggested a few minor updates in a new PR, https://github.com/epiforecasts/scoringutils/pull/788.
One conceptual question: Should customise_metric()
produce a function that accepts ...
as args? In some sense, the whole purpose of the function is to make any calls to ...
explicit.
In some cases, this could produce an error:
custom_metric <- customise_metric(mean, na.rm = TRUE)
# Use the customised metric function
values <- c(1, 2, NA, 4, 5)
custom_metric(values, na.rm = TRUE)
Since we're now ignoring the ...
in score(forecast, metrics, ...)
it doesn't make much of a difference at the moment. So probably more of a philosophical question.
Other than that we should add some explanation of this to the manuscript and vignette. I created a new issue for that (#789).
I suggested a few minor updates in a new PR, https://github.com/epiforecasts/scoringutils/pull/788.
Looks good.
Should customise_metric() produce a function that accepts ... as args? In some sense, the whole purpose of the function is to make any calls to ... explicit.
It seems like any other solution here would require quite a lot of coding to only support an edge case that is clearly not documented/suggested?
The pattern this is all following is purrr::partial
which we could wrap directly (we would want a wrap as we might want to change to having a scoring rules class in the future and this approach would prevent UI changes) but I'm not sure we really need to do so?
It seems like any other solution here would require quite a lot of coding to only support an edge case that is clearly not documented/suggested?
We could just remove the list(...)
in customise_metric()
and I think that wouldn't necessitate any other changes (as we're not using the ...
anywhere at the moment if I'm not mistaken).
But if we're mirroring purrr::partial()
then that's fine with me. Happy to not wrap it for now - whatever you think is best.
Ah I messed up by introducing a |>
in the test...
Otherwise looks good.
We could just remove the list(...) in customise_metric() and I think that wouldn't necessitate any other changes (as we're not using the
I doon't think that will work? The ...
in list() is what hard codes the arguments into the internal function generator whilst the ...
in function() is just saying the function that is generated can take any arguments?
I doon't think that will work? The
...
in list() is what hard codes the arguments into the internal function generator whilst the...
in function() is just saying the function that is generated can take any arguments?
I meant in here do.call(metric, c(list(...), dots))
. I don't think we ever use the ...
. but I might be wrong
Edit: what I meant is that the function maybe shouldn't take any additional arguments, because any arguments it could take should have been already incorporated when producing the custom function. But again not a very strong opinion and happy to merge as is.
I meant in here do.call(metric, c(list(...), dots)). I don't think we ever use the .... but I might be wrong
This is what let's you pass the actual required input for the metric at run time vs at modification? i.e the forecasts and actuals? The only reason to do it as ...
vs naming them is so that you can use this for any kind of metric/anything that might have different standard input args
Oh... Right... Maybe I don't deserve that PhD yet.... 👀
Description
This PR closes #431 by stopping passing arguments to underlying scoring rules and providing a new helper function (
customise_metric
) to make it easier to manipulate the metric list. I think this makes a lot of sense as a workflow as in2.0.0
we are focussing on the metric list being the point of customisation and score has become something that just runs that against data. This change will I think encourage people to look at that list.I tested this by removing the
...
arg from all functions and checking what broke (and then fixing it). I found no documented uses of the...
passing or documentation discussing it.Note that I left in the
apply_metric
functionality for this should we get a strong user demand.Checklist
lintr::lint_package()
to check for style issues introduced by my changes.