Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #494: New workflow for creating and validating forecast objects #531

Merged
merged 12 commits into from
Dec 19, 2023

Conversation

nikosbosse
Copy link
Contributor

@nikosbosse nikosbosse commented Dec 16, 2023

Description

This PR closes #494.

This PR

  • replaces the function validate.default() with a new class construction helper as_forecast(), a function that determines the forecast type, constructs a new class object, and validates that object. as_forecast() is a generic with a single method as_forecast.default() that does the work
  • Replaces all instances of validate() (in the sense of the previous validate.default()) in the code, the news file, and the tests
  • Reorganises the documentation for score() and as_forecast() a bit, such that a section that is common to both can be reused
  • adds the .trackdown folder to .gitignore and .buildignore - the changes suggested in Implement updated validation and class construction workflow #494, but I recently used trackdown on a different branch (but also gitignored it so I can't just delete the file) for updates to the manuscript
  • Renamed the generic validate() to validate_forecast(). The class construction helper (which was previously validate.default() is therefore now as_forecast() and the validation function (previously the methods of validate()) are now methods of validate_forecast()

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.

Copy link

codecov bot commented Dec 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c11ce14) 82.44% compared to head (f7ece21) 82.47%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #531      +/-   ##
===========================================
+ Coverage    82.44%   82.47%   +0.03%     
===========================================
  Files           20       20              
  Lines         1675     1678       +3     
===========================================
+ Hits          1381     1384       +3     
  Misses         294      294              

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

@nikosbosse nikosbosse requested a review from seabbs December 16, 2023 23:48
NAMESPACE Show resolved Hide resolved
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me. Confused about the use of scoringutils_binary etc. and I think could do with a bit more checking the as_forecast workflow makes sense and is used clearly.

R/validate.R Show resolved Hide resolved
R/validate.R Show resolved Hide resolved
README.Rmd Show resolved Hide resolved
vignettes/scoringutils.Rmd Outdated Show resolved Hide resolved
vignettes/scoringutils.Rmd Show resolved Hide resolved
@seabbs
Copy link
Contributor

seabbs commented Dec 19, 2023

Can you resolve and request another review when ready please!

@nikosbosse
Copy link
Contributor Author

I updated score.default() to not validate forecasts. I also updated the Vignette to make use set_forecast_unit() and as_forecast() consistently.

We'll likely need another round of edits for the Vignette. I suggest to not mix that with this PR. I haven't finished updating the scoringutils manuscript yet and I think we can re-use parts of that, creating a coherent structure that makes sense. I opened #532 to keep track of this.

Side note: I also made a new comment to #435 since I noticed that the current default for by in summarise_scores() maybe doesn't that make much sense anymore now that we're not returning scores per quantile anymore.

PR is good for another round of review. Thanks a lot!

@nikosbosse nikosbosse requested a review from seabbs December 19, 2023 12:28
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. All makes sense. Looks good to me.

@seabbs seabbs merged commit 0b01113 into develop Dec 19, 2023
11 checks passed
@seabbs seabbs deleted the rework-validate branch December 19, 2023 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants