Closed nikosbosse closed 5 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 95.30%. Comparing base (
890e6cf
) to head (6e5b451
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
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 silentvalidate_forecast
calls assert_forecast
and returns it (this is user facing for piping etc.clean_forecast
na.omit
and a copy. Internal only? 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?
I like that
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?
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?
Shall we merge this in and address validate_forecast
again in another issue?
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:
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...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? 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.
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.
Yes I think so. Why can't validate_forecast call assertforecast (and hence have no duplication)? I also think we can not export `assert` if we want or at least for nwo
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.
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.
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?
main
and figuring out merge conflictsno real preference on approach. whatever works/is simplest
I'll merge this now then and make a new PR
Description
As discussed in #447, some functions lack good input checks. This is the second PR that adds more input checks.
This PR
plot_pairwise_comparisons()
- Note: should we rename the first argument fromcomparison_result
topairwise
?plot_pit()
validate_forecast_internal()
allows creating a copy and omittingNA
rows (as discussed in #688).forecast
objecttransform_forecasts()
get_coverage()
get_pit()
score()
.Checklist
lintr::lint_package()
to check for style issues introduced by my changes.