Issue #848 - Split `as_forecast()` into separate functions for the different forecast types

[nikosbosse]

Description

This PR closes #848.

This PR

• splits up as_forecast() into separate functions
• moves the part that's common to all as_forecast functions to an internal as_forecast_generic (similar to what we do with assert_forecast
• updates docs and the manuscript

Notes:

• I'm sometimes referring to the collective of the as_forecast() functions as as_forecast_...() which is a bit awkward. Do we like something like as_forecast_[forecast_type]() better?

Checklist

• [x] My PR is based on a package issue and I have explicitly linked it.
• [x] I have included the target issue or issues in the PR title as follows: issue-number: PR title
• [x] I have tested my changes locally.
• [x] I have added or updated unit tests where necessary.
• [x] I have updated the documentation if required.
• [x] I have built the package locally and run rebuilt docs using roxygen2.
• [x] My code follows the established coding standards and I have run lintr::lint_package() to check for style issues introduced by my changes.
• [x] 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.

[seabbs]

Do we like something like asforecast[forecast_type]() better?

Yes I think so or as_forecast_<type> maybe

[nikosbosse]

A few questions about if we want a forecast meta class or not.

were there any questions I haven't addressed yet?

Also do we need class specific is_forecast_sample etc vs doing is_forecast.forecast_sample?

I think so. My rationale would be that you want to be able to check whether something is a forecast object and also whether it is a forecast object of a specific kind. But no strong opinion here. @Bisaloo do you have thoughts?

I changed the name of as_forecast_...() to as_forecast_<type>. If I didn't forget anything than the PR is good for another look from you.

[Bisaloo]

I think so. My rationale would be that you want to be able to check whether something is a forecast object and also whether it is a forecast object of a specific kind. But no strong opinion here. @Bisaloo do you have thoughts?

I think you're right. This is more a workflow question than a technical question but I expect situations where we want to know the specific type of the forecast object.

[nikosbosse]

I'm assuming @seabbs response would be that you could always call the specific method (e.g. is_forecast.forecast_binary) explicitly. Which is true, but also people don't generally seem to be very fond of calling specific methods directly

[Bisaloo]

I'm assuming @seabbs response would be that you could always call the specific method (e.g. is_forecast.forecast_binary) explicitly.

No, this is different.

is_forecast.forecast_binary() will tell you if an object is of class forecast, but it won't tell you if it's a forecast_binary.

Actually, you are not supposed to call methods directly. It defeats the purpose of OOP and S3 dispatch. As such, is_forecast.forecast_binary() would always return TRUE as forecast_binary is a child class of forecast.

[nikosbosse]

Makes sense! I think then I'd vote for keeping as is

[seabbs]

Makes sense! I think then I'd vote for keeping as is

I am bit confused by this conversation and the actions that are linked to it. I think we all agree that we want but don't have the following:

is_forecast : Checks something has class forecast is_forecast_<type> checks the specific type.

Am I correct in that assumption? If yes I think we need a new issue if no action happening here. If no then a new issue as well probably?

[nikosbosse]

I am bit confused by this conversation and the actions that are linked to it. I think we all agree that we want but don't have the following:

is_forecast : Checks something has class forecast isforecast checks the specific type.

Maybe the confusion is solved by the fact that we already have these, currently implemented in the following way:

is_forecast <- function(x) {
  inherits(x, "forecast")
}

#' @export
#' @rdname is_forecast
#' @keywords check-forecasts
is_forecast_sample <- function(x) {
  inherits(x, "forecast_sample") && inherits(x, "forecast")
}

#' @export
#' @rdname is_forecast
#' @keywords check-forecasts
is_forecast_binary <- function(x) {
  inherits(x, "forecast_binary") && inherits(x, "forecast")
}

etc.

[nikosbosse]

@seabbs do you think this is good to merge?