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

Multiple issues: metric -> rule #600

Closed
wants to merge 4 commits into from
Closed

Multiple issues: metric -> rule #600

wants to merge 4 commits into from

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Jan 12, 2024

Description

Renames metric -> rule across the package via full text search.

Partly addresses #476 (manuscript is untouched).
Partly addresses #589 (default behaviour is untouched).
Partly addresses #401 (documentation is untouched).

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 Jan 12, 2024

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (c523ee1) 85.53% compared to head (e527ddb) 85.53%.

Files Patch % Lines
R/pairwise-comparisons.R 69.56% 7 Missing ⚠️
R/summarise_scores.R 50.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #600   +/-   ##
=======================================
  Coverage   85.53%   85.53%           
=======================================
  Files          21       21           
  Lines        1721     1721           
=======================================
  Hits         1472     1472           
  Misses        249      249           

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

@nikosbosse
Copy link
Contributor

nikosbosse commented Jan 12, 2024

Thanks a lot for tackling this one! The naming really is a bit of a mess at the moment, unfortunately (and so are the issues that talk about it).

We should probably make one of them the ground truth, maybe #520. There, the explanation is

We call "scoring rule" any function that computes a score / metric / quantity of interest based on forecasts and observed values. We call "score" the output of a scoring rule.

"Scoring rule" usually means the specific R function that computes a score. We use sentences such as "you can pass a list of scoring rules to score(), or "you can call a scoring rule directly". And "scores are computed by scoring rules". Sometimes, "scoring rule" can also denote the function in a more abstract, mathematical sense. For example we would say "the absolute error is a scoring rule" or the "CRPS" is a proper scoring rule. Sometimes we use "rules" as short for "scoring rules", for example when naming function arguments.

In other contexts in the literature, "score" is also used as short for "scoring rule", for example in a sentence like "the CRPS is a proper score". We will avoid this terminology.

So rule or scoring rule would be the function (or the name for the mathematical concept) and score would be the output of a scoring rule.

In #588 we named the function get_score_names(). In that sense, "CRPS" would be name of the score (the output of the scoring rule) as well as the name of the scoring rule itself. I think it would be consistent to call the arguments in correlation() and add_pairwise_comparison() score_name/score_names. What do you think?

Edit: we could also just run away from the problem and call it which

@sbfnk
Copy link
Contributor Author

sbfnk commented Jan 17, 2024

In that sense, "CRPS" would be name of the score (the output of the scoring rule) as well as the name of the scoring rule itself.

I'm not convinced this distinction is helpful. I would think "the name of the scoring rule" can be interpreted as the same as "the scoring rule". Why not rename get_score_names() to get_scoring_rules() and then have rules in correlation() and add_pairwise_comparison()?

I would prefer calling everything scoring_rules instead of rules but that is perhaps another discussion.

@seabbs
Copy link
Contributor

seabbs commented Jan 18, 2024

I'm not convinced this distinction is helpful. I would think "the name of the scoring rule" can be interpreted as the same as "the scoring rule".

The place where this might be needed is when a scoring rule has multiple outputs so WIS decomposition or different coverage levels. Having a different name for the output vs the rule does help distinguish those. I agree though that these are edge cases and we want to avoid things getting too complicated. Personally, I just want to pick something and stick with it. Happy to use rules for the function/mathematical concept and score for output or call everything scoring rule/rule for short.

Edit: we could also just run away from the problem and call it which

No 😱 please no.

I would prefer calling everything scoring_rules instead of rules but that is perhaps another discussion.

I agree that is another discussion and one we could revisit but ideally not here. There is an issue floating around where this was discussed I think. We ended up on rule because score we being used very repetitively i.e score(scoring_rules = list(scoring_rules...)) which felt quite redundant.

@nikosbosse
Copy link
Contributor

I... made another... issue for naming things... #610 🥲

@seabbs
Copy link
Contributor

seabbs commented Jan 22, 2024

😭

@nikosbosse
Copy link
Contributor

As discussed in #610 my current erratic best take is to call the argument metric after all. Do you agree, @sbfnk? I'm going to close this PR for now

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.

None yet

3 participants