Issue 432: Add some basic functions for as_point and as_quantile #790

seabbs · 2024-04-15T18:20:07Z

Description

This PR closes #432. It adds basic s3 methods for as_point and as_quantile to map from sample and quantile forecasts to quantile and point forecasts.

It also adds an option to as_forecast to skip checks as these are a bit annoying and adds an internal function remake_forecast to reclass a forecast and then use assert_forecast. Potentially we may want to expose the latter.

Do we have an issue for refactoring as_forecast.default in a later release as the current monolithic approach will probably make user class extensions hard/annoying.

@pearsonca this is a good example of having to reinvent the wheel quantile wise (i.e your epinowcast community post)

Note: Several of these implementations could be more efficient but I see this as putting the groundwork and I think improving them etc. are topics for their own issues.

Checklist

My PR is based on a package issue and I have explicitly linked it.
I have included the target issue or issues in the PR title as follows: issue-number: PR title
I have tested my changes locally.
I have added or updated unit tests where necessary.
I have updated the documentation if required.
I have built the package locally and run rebuilt docs using roxygen2.
My code follows the established coding standards and I have run lintr::lint_package() to check for style issues introduced by my changes.
I have added a news item linked to this PR.
I have reviewed CI checks for this PR and addressed them as far as I am able.

sbfnk

Looks good - would have expected perhaps also to see an as_quantile.forecast_point (setting the 0.5 quantile to the point forecast)?

sbfnk · 2024-04-16T16:05:36Z

R/as_point.R

+#' @param quantile_level The desired quantile level for the point forecast.
+#' Defaults to 0.5 (median).


Surprised to see this as an argument - do you have a use case in mind where one would want it not to be 0.5?

Not reallly but I thought there was little issue with giving this option

Agree that it's a minor issue but it does add to the cognitive burden on anyone coming tot he function so if we think there's no use case it might be better to simplify?

R/as_point.R

R/as_quantile.R

R/forecast.R

R/as_point.R

nikosbosse

Thanks! This is cool functionality to have. I left a few comments. My main points are the duplication of the as_quantile() function for sample-based forecasts and the changes to as_forecast() which I'm not sure I completely understand.

NAMESPACE

R/as_point.R

R/as_quantile.R

R/forecast.R

nikosbosse · 2024-04-17T07:50:16Z

R/forecast.R

+#'
+#' @return The modified forecast object
+#' @keywords internal
+remake_forecast <- function(


Alternatively, we could just call as_forecast() instead. The current sample_to_quantile() function does that.
I'd be happy to avoid the additional code complexity of introducing a new function.

I tried to do this hence the new checking option and it still wouldn't work. This what motivated the question about refactoring as_forecast as the current approach with the type specific checking really doesn't seem feasible for future extensions.

I think we should make a new issue for this (related to the as_forecast issue). There is also a more general point about whether or not as_forecast should be able to reclass a forecast object or whether it does actually make sense workflow wise to have something that explicitly removes and then readds the class.

nikosbosse · 2024-04-17T07:52:31Z

R/forecast.R

+  forecast, old_classname, new_classname, verbose = TRUE
+) {
+  remade_forecast <- forecast
+  class(remade_forecast) <- setdiff(class(forecast), old_classname)


new_forecast() calls as.data.table() which strips the class anyway

I tried this and was having some weird errors. Can you confirm? More generally I think its helpful to make this explicit in the code.

Hmm. I'll check it out and play around a bit with this. Circling back!

nikosbosse · 2024-04-17T07:55:21Z

R/summarise_scores.R

@@ -60,13 +62,17 @@ summarise_scores <- function(scores,
                             by = "model",
                             across = NULL,
                             fun = mean,
+                             metrics,


I'm not sure we strictly need an additional argument given that users can specify the metrics in score().

If we decide to have it then I'd vote for making the default metrics = get_metrics(scores).

What does get_metric do if the input isn't a score object? That was the rub here.

I only added this so I can could use summarise_scores on more general objects and save introducing more code.

More generally I also quite like the idea that naive users can reduce their score object at this point as many likely won't know to manipulate the metric list going into score regardless of docs.

All of above being said I think this is a new issue to discuss.

Maybe it makes sense not to use summarise_scores() here (at least for now).

summarise_scores() ultimately is a glorified one-liner and the actual work happens here:

scores <- scores[, lapply(.SD, fun, ...), by = c(by), .SDcols = colnames(scores) %like% paste(metrics, collapse = "|") ]

Then we could discuss the metrics argument in a separate issue/PR.

seabbs · 2024-04-17T09:19:42Z

would have expected perhaps also to see an as_quantile.forecast_point (setting the 0.5 quantile to the point forecast)?

Given the original issue was just for as_point I think this might be a new feature request.

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

Co-authored-by: Nikos Bosse <37978797+nikosbosse@users.noreply.github.com>

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

seabbs · 2024-04-17T09:37:05Z

My main points are the duplication of the as_quantile() function for sample-based forecasts and the changes to as_forecast() which I'm not sure I completely understand.

Hoopefully the responses cover the rational here and the proposed actions.

…oint'

Bisaloo · 2024-04-18T15:47:33Z

NAMESPACE

+S3method(as_point,default)
+S3method(as_point,forecast_quantile)
+S3method(as_point,forecast_sample)
+S3method(as_quantile,default)
+S3method(as_quantile,forecast_sample)


Only minor comment is that this feels as if there is a new point, or quantile class. I believe it would make sense, and be slightly easier to wrap my head around as a potential user, if we re-used the existing classes and terminology. I.e., forecast_point and forecast_quantile.

This would lead to longer names but that's probably still ok 🤔

Yeah I think if we are going with a abstract class as in #373 we should rename.

NEWS.md

R/forecast.R

R/as_point.R

nikosbosse · 2024-04-19T07:55:00Z

R/as_point.R

+#' # Function approach
+#' as_point(sample_forecast, fun = mean)
+as_point.forecast_sample <- function(forecast, quantile_level = 0.5, fun, ...) {
+  assert_forecast(forecast, verbose = FALSE)


A new issue sounds good. If we're keeping the assert_forecast() then I think we definitely want it here.

The bigger question is probably "do we want to do a validation check at all?" and I think that's up for debate.
But if we're doing a validation check then we should also check whether the object has the right forecast type.

R/as_quantile.R

nikosbosse · 2024-04-19T07:57:40Z

R/as_point.R

+#' @examples
+#' as_point(as_forecast(example_quantile))
+as_point.forecast_quantile <- function(forecast, quantile_level = 0.5, ...) {
+  assert_forecast(forecast, verbose = FALSE)


Suggested change

assert_forecast(forecast, verbose = FALSE)

assert_forecast(forecast, forecast_type = "quantile", verbose = FALSE)

If we validate the forecast at all then we should also make sure the forecast type is correct.

Seee above point and my disagreement with this

nikosbosse · 2024-04-19T08:03:04Z

R/forecast.R

+  forecast, old_classname, new_classname, verbose = TRUE
+) {
+  remade_forecast <- forecast
+  class(remade_forecast) <- setdiff(class(forecast), old_classname)


Hmm. I'll check it out and play around a bit with this. Circling back!

nikosbosse · 2024-04-19T08:05:44Z

R/summarise_scores.R

@@ -60,13 +62,17 @@ summarise_scores <- function(scores,
                             by = "model",
                             across = NULL,
                             fun = mean,
+                             metrics,


Maybe it makes sense not to use summarise_scores() here (at least for now).

summarise_scores() ultimately is a glorified one-liner and the actual work happens here:

scores <- scores[, lapply(.SD, fun, ...), by = c(by), .SDcols = colnames(scores) %like% paste(metrics, collapse = "|") ]

Then we could discuss the metrics argument in a separate issue/PR.

nikosbosse · 2024-04-19T08:08:02Z

R/utils_data_handling.R

-#' @export
-#' @examples
-#' sample_to_quantile(as_forecast(example_sample_discrete))
+#' @keywords internal
 sample_to_quantile <- function(forecast,


Shouldn't this function be deleted then?

I think it can be but this PR is getting widely out of scope hence just making it internal in the first instance

nikosbosse · 2024-04-19T08:11:14Z

R/as_quantile.R

+#' @examples
+#' as_quantile(as_forecast(example_sample_continuous))
+as_quantile.forecast_sample <- function(
+  forecast, quantile_levels = seq(from = 0.01, to = 0.99, by = 0.01), ...


Suggested change

forecast, quantile_levels = seq(from = 0.01, to = 0.99, by = 0.01), ...

forecast, quantile_level = seq(from = 0.01, to = 0.99, by = 0.01), ...

I vote for calling this quantile_level for two reasons:

the argument is called quantile_level everywhere else

the column in the output object will also be called quantile_level
I see the point for using the plural, but overall think it would be easier to use the singular here.

I don't agree but happy to standardise with the rest of the code base. It feels like this needs a new issue though as its not very intuitive. (i.e its probs not prob in quantile for a reason).

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

Co-authored-by: Nikos Bosse <37978797+nikosbosse@users.noreply.github.com>

seabbs requested a review from nikosbosse April 15, 2024 18:20

seabbs force-pushed the issue432 branch from 831db9e to 665d938 Compare April 15, 2024 18:21

seabbs changed the title ~~Issue 432add some basic functions for as_point and as_quantile~~ Issue 432: Add some basic functions for as_point and as_quantile Apr 15, 2024

seabbs force-pushed the issue432 branch from 665d938 to 0115c21 Compare April 15, 2024 19:48

seabbs added 6 commits April 15, 2024 20:52

add some basic functons for as_point and as_quantile

a9c180e

add documentation for as_point and as_quantile

bc21d3e

add a news item and changes for CRAN check

bb3c8dd

add simple tests

c5cf871

clean up linting

e64aae9

add remake_forecast internal flag for pkgdown

112fe32

seabbs force-pushed the issue432 branch from 45ab9f7 to 112fe32 Compare April 15, 2024 19:52

seabbs enabled auto-merge (squash) April 15, 2024 19:59

seabbs and others added 2 commits April 15, 2024 21:07

drop not very good remake_forecast tests

e4a43c7

Update NEWS.md

dd23db3

sbfnk reviewed Apr 16, 2024

View reviewed changes

nikosbosse requested changes Apr 17, 2024

View reviewed changes

seabbs and others added 5 commits April 17, 2024 10:22

Update R/as_quantile.R

82fd28d

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

Update forecast.R

0791871

Update R/forecast.R

e13dfd0

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

Update R/as_point.R

6fd8962

Co-authored-by: Nikos Bosse <37978797+nikosbosse@users.noreply.github.com>

Update R/as_point.R

90d17a4

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

seabbs added 2 commits April 17, 2024 10:54

fix broken PR suggests

82a36f3

make sample_to_quantile internal and remove quantile_level from 'as_p…

6be7724

…oint'

seabbs requested review from nikosbosse and sbfnk April 17, 2024 10:45

linting and catch public use of sample_to_quantile

737f4e9

seabbs mentioned this pull request Apr 18, 2024

Review scoringutils 2.0.0 #791

Merged

Bisaloo reviewed Apr 18, 2024

View reviewed changes

nikosbosse mentioned this pull request Apr 19, 2024

How should we document methods that have different arguments than the default method? #793

Open

sbfnk reviewed Apr 19, 2024

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

R/forecast.R Outdated Show resolved Hide resolved

nikosbosse reviewed Apr 19, 2024

View reviewed changes

seabbs and others added 3 commits April 19, 2024 10:04

Update NEWS.md

5fae245

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

Update R/forecast.R

c2b62f3

Co-authored-by: Sebastian Funk <sebastian.funk@lshtm.ac.uk>

Update R/as_point.R

6688f2f

Co-authored-by: Nikos Bosse <37978797+nikosbosse@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 432: Add some basic functions for as_point and as_quantile #790

Issue 432: Add some basic functions for as_point and as_quantile #790

seabbs commented Apr 15, 2024 •

edited

sbfnk left a comment

sbfnk Apr 16, 2024

seabbs Apr 17, 2024

sbfnk Apr 17, 2024

nikosbosse left a comment

nikosbosse Apr 17, 2024

seabbs Apr 17, 2024

nikosbosse Apr 17, 2024

seabbs Apr 17, 2024

nikosbosse Apr 19, 2024

nikosbosse Apr 17, 2024

seabbs Apr 17, 2024

nikosbosse Apr 19, 2024

seabbs Apr 19, 2024

seabbs commented Apr 17, 2024

seabbs commented Apr 17, 2024

Bisaloo Apr 18, 2024

seabbs Apr 19, 2024

nikosbosse Apr 19, 2024

nikosbosse Apr 19, 2024

nikosbosse Apr 19, 2024

seabbs Apr 19, 2024

nikosbosse Apr 19, 2024

nikosbosse Apr 19, 2024

nikosbosse Apr 19, 2024

seabbs Apr 19, 2024

nikosbosse Apr 19, 2024

seabbs Apr 19, 2024

		#' @param quantile_level The desired quantile level for the point forecast.
		#' Defaults to 0.5 (median).

	assert_forecast(forecast, verbose = FALSE)
	assert_forecast(forecast, forecast_type = "quantile", verbose = FALSE)

	forecast, quantile_levels = seq(from = 0.01, to = 0.99, by = 0.01), ...
	forecast, quantile_level = seq(from = 0.01, to = 0.99, by = 0.01), ...

Issue 432: Add some basic functions for as_point and as_quantile #790

Are you sure you want to change the base?

Issue 432: Add some basic functions for as_point and as_quantile #790

Conversation

seabbs commented Apr 15, 2024 • edited

Description

Checklist

sbfnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikosbosse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seabbs commented Apr 17, 2024

seabbs commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seabbs commented Apr 15, 2024 •

edited