Skip to content

Conversation

@nikosbosse
Copy link
Collaborator

@nikosbosse nikosbosse commented Oct 23, 2023

This PR

  • replaces check_forecasts() by a new function, validate() which assigns the class on an input without a class and also serves as the validator function for the scoringutils_binary, scoringutils_point, scoringutils_quantile and scoringutils_sample classes
  • Creates S3 methods for score(), but as of now leaves scoringutils_quantile() untouched. This one is scary. And maybe can be it's own PR in the spirit of my (already failed) attempt to partition things into smaller PRs.
  • reorganises the input check functions a bit (trying to make them consistent between check_, assert_ and test_)
  • reorganises functions into different files, one for binary metrics, sample metrics etc...

nikosbosse and others added 30 commits October 23, 2023 14:57
…heck_data_columns that check input is data.table with right columns
…e reason, but plots are otherwise still the same.
@nikosbosse nikosbosse marked this pull request as ready for review October 26, 2023 13:52
@nikosbosse nikosbosse requested a review from seabbs October 26, 2023 14:16
@nikosbosse nikosbosse mentioned this pull request Oct 30, 2023
@seabbs
Copy link
Contributor

seabbs commented Nov 1, 2023

reorganises the input check functions a bit (trying to make them consistent between check_, assert_ and test_)
reorganises functions into different files, one for binary metrics, sample metrics etc...

In an ideal world these changes would be there own PR as it adds a lot of noise to this one.

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.

Because of how much is loaded intoo this PR its a real nightmare to review. I would suggest addressing the things mentioned in earlier PRs in those PRs and saving moving files around etc for their own PRs.

I will circle back and address more fully. Current review comments flag issues linting and issues with the setup of score and the naming of validate

score <- function(data,
metrics = NULL,
...) {
score <- function(data, ...) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is metrics not an arg for score?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't quite sure what the default should be. No default (but then again the default is in the methods)? NULL? I was hoping it would be more clear that way when looking at the documentation for the methods.

Base automatically changed from rework-lower-level-fcts to scoringutils-review November 1, 2023 16:53
@nikosbosse nikosbosse merged commit f1b9388 into scoringutils-review Nov 7, 2023
@nikosbosse
Copy link
Collaborator Author

Merging after reviewing and discussing sesssions

@nikosbosse nikosbosse deleted the rework-score() branch November 7, 2023 11:58
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