Issue #447 - Add and simplify more input checks

[nikosbosse]

Description

As discussed in #447, some functions lack good input checks. This is the second PR that adds more input checks.

This PR

• adds input checks to
  • plot_pairwise_comparisons() - Note: should we rename the first argument from comparison_result to pairwise?
  • plot_pit()
• adds a new internal helper function to simplify input checks: validate_forecast_internal() allows creating a copy and omitting NA rows (as discussed in Roles of as_forecast() and validate_forecast() #688).
• Applies this helper function to all functions that depend on a forecast object
• transform_forecasts()
• get_coverage()
• get_pit()
• score().

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.30%. Comparing base (890e6cf) to head (6e5b451).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #753      +/-   ##
==========================================
- Coverage   95.31%   95.30%   -0.02%     
==========================================
  Files          21       21              
  Lines        1602     1598       -4     
==========================================
- Hits         1527     1523       -4     
  Misses         75       75              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sbfnk]

I must admit I don't see the value of creating validate_forecasts_internal. It just adds overhead, and as an unsuspecting developer I would expect a validate...() to just perform checks but not manipulate an object.

[seabbs]

as implemented neither of copy or na.omit are ever not true. If we are doing this then why not make these the defaults?

[seabbs]

All changes aside from the validate_forecast stuff are good to go.

In terms of that what about if we had:

• assert_forecast does checks and returns nothing - has a silent
• validate_forecast calls assert_forecast and returns it (this is user facing for piping etc.
• clean_forecast na.omit and a copy. Internal only?

[seabbs]

so most uses here would be:

forecast <- clean_forecast(forecast)
assert_forecast(forecast, verbose = FALSE)

and in the wider world you would do things like:

forecast |>
  validate_forecast()
  score()

if you didn't want to copy/convert in as_forecast and thought you already had a forecast object?

[nikosbosse]

I like that

[nikosbosse]

Only requires some thinking related to the S3 methods. Then, it seems, that validate_forecast() would just be a regular function and assert_forecast() becomes an S3 method.

This means we would export all three of them, right?

[seabbs]

Only requires some thinking related to the S3 methods. Then, it seems, that validate_forecast() would just be a regular function and assert_forecast() becomes an S3 method.

Yes I think so?

Wouldn't we only export validate_forecast and maybe assert_forecast?

[seabbs]

Shall we merge this in and address validate_forecast again in another issue?

[seabbs]

LGTM

[nikosbosse]

I've been going in circles a bit in my head. Latest change: I renamed validate_forecast_internal() to clean_forecast() to at least address Seb's point that he'd be confused by a validation function doing copies and other stuff.

Further thoughts:

• There currently is still a call to validate_forecast() in clean_foreast(). We could move that outside (that was what Sam suggested. Pro: maybe cleaner. Con: Then again the only two things that clean_forecast() does is a) call na.omit() and b) call copy(). I'm not sure replacing two lines with a new function makes that much sense...
• We had an argument about whether validate_forecast() should return the data or invisble(NULL). I think there is potentially a case for both (invisible(NULL) feels maybe? cleaner, returning the data makes it pipeable). I am, however, not so sure anymore that exporting both functions really is the answer. Maybe we should stick to one?

[seabbs]

feels maybe? cleaner, returning the data makes it pipeable). I am, however, not so sure anymore that exporting both functions really is the answer. Maybe we should stick to one?

Isn't this assert_forecast.

does is a) call na.omit() and b) call copy(). I'm not sure replacing two lines with a new function makes that much sense.

I think replacing two lines of repeated code across functions with an internal function makes a huge amount of sense.

[nikosbosse]

feels maybe? cleaner, returning the data makes it pipeable). I am, however, not so sure anymore that exporting both functions really is the answer. Maybe we should stick to one?

Isn't this assert_forecast.

Yes this would be assert_forecast(). This other one would be validate_forecast(). But do we want to have two functions that do practically the same thing? (three if you count as_forecast()). We could alternatively just pick one.

[seabbs]

Yes I think so. Why can't validate_forecast call assert_forecast (and hence have no duplication)? I also think we can not export assert_ if we want or at least for nwo

[nikosbosse]

Yes I think so. Why can't validate_forecast call assert_forecast (and hence have no duplication)?

Yes one should definitely call the other. I'm less concerned about code duplication, but more concerned about exporting a lot of similar functions to the user.

I also think we can not export assert_ if we want or at least for now

If validate_forecast() calls assert_forecast() then we must export both, because assert_forecast() would be the new S3 method which by default is always exported. Also assert_forecast() would then be the entry point for someone who wants to extend the functionality for new classes.

[seabbs]

It looks like you can keep it internal if you want with a bit fo leg work: https://stackoverflow.com/questions/68515108/internal-s3-generics-with-an-lapply

To be honest I am really not clear what the issue is with exporting both as they both have very clear differences? If anything we could get rid of validate_forecast and stick with just assert_ but I also think we have had that conversation several times and decided the pipe workflow is good to have.

[nikosbosse]

To be honest I am really not clear what the issue is with exporting both as they both have very clear differences?

Then let's do that.

Any preferences re either of the following options?

1. merging this + new PR for the implementation
2. new PR with the implementation targeting this branch
3. new PR with the implementation targeting main and figuring out merge conflicts
4. other

[seabbs]

no real preference on approach. whatever works/is simplest

[nikosbosse]

I'll merge this now then and make a new PR