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 #832 - Make default metrics function S3 #903

Merged
merged 8 commits into from
Sep 16, 2024
Merged

Conversation

nikosbosse
Copy link
Contributor

@nikosbosse nikosbosse commented Sep 10, 2024

Description

This PR closes #832.

Creates a single get_metrics() generic with S3 methods for forecast and scores objects.

Additional comments:

  • still need to update the news file again
  • could create a default method that just says "error, needs a forecast or scores object". But then again R already warns you that there is no appropriate method
  • I'm planning to move all functions to respective files for the different forecast types. This should make adding more forecast types in the future easier. But I thought it would be easier for review to leave as is for now and tackle it in a different PR.

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.

@nikosbosse nikosbosse changed the title Make default metrics function S3 Issue #832 - Make default metrics function S3 Sep 12, 2024
@nikosbosse nikosbosse marked this pull request as ready for review September 16, 2024 11:26
@nikosbosse nikosbosse requested a review from seabbs September 16, 2024 11:26
@seabbs
Copy link
Contributor

seabbs commented Sep 16, 2024

But I thought it would be easier for review to leave as is for now and tackle it in a different PR.

Good call

@seabbs seabbs enabled auto-merge (squash) September 16, 2024 12:26
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.

I really like how this has turned out. I can think of a few uses cases where this might annoy people (if they don't have an object of the right class but want the list) but think that overall should be a much better user experience.

@seabbs seabbs merged commit 656d817 into main Sep 16, 2024
9 checks passed
@seabbs seabbs deleted the rework-default-metrics branch September 16, 2024 12:30
@nikosbosse
Copy link
Contributor Author

I can think of a few uses cases where this might annoy people (if they don't have an object of the right class but want the list)

yeah me too. In that sense I think changing the examples to pre-validated was a good move

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.

metrics_ -> metrics.scoringutils.quantile etc
2 participants