Issue #474: Make default scoring rules functions rather than stored data sets

[nikosbosse]

Description

This PR fixes #474.

It

• replaces the previous data sets (metrics_point, metrics_binary, metrics_quantile and metrics_sample) with functions that return a list with functions used as default scoring rules (rules_point(), rules_binary(), rules_quantile() and rules_sample())
• introduces a new helper function, select_rules() to implement basic functionality to select or exclude functions from the default list in a call to rules_*()
• updates existing tests and adds new tests for the new functions
• replaces a previous \(...) with function(...) to make sure everything can be run on R 3.6
• updates the NEWS file

Further considerations:

• This PR does not yet resolve all naming consistencies (i.e. "metrics" and "rules") see Implement consistent naming and language for talking about scoring rules #476. I suggest addressing this separately.
• The PR replaces existing functionality. We talked about additional features such as making scoring rules modular and composable, e.g. by creating a function like rules_wis() that includes all WIS rules.
• In Documentation: Add more documentation + print method for default metrics #365 and Rethink metrics table #415 we discuss additional ways to document default scoring rules.
  • From previous versions there still is a data object, metrics, which is a data.table of explanations. I think we should ultimately get rid of this
  • We previously talked about a print() method for the scoring rules with further explanations. I'm not convinced anymore we really need this. The documentation for rules_*() links to the documentation for the additional functions, which should have all the explanations the user needs. In addition, users should be able to find everything in the vignettes. Are we happy with this? If so I suggest deleting the stored metrics() object.
  • We could, however, have a print() method that simply adds a sentence "the following scores will be computed: 'names of scores'" before printing the list. This could be nice, but maybe not necessary

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d04a82c) 82.50% compared to head (890ab29) 83.73%.
Report is 18 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #536      +/-   ##
===========================================
+ Coverage    82.50%   83.73%   +1.23%     
===========================================
  Files           20       21       +1     
  Lines         1680     1722      +42     
===========================================
+ Hits          1386     1442      +56     
+ Misses         294      280      -14     

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

[seabbs]

Are we happy with this? If so I suggest deleting the stored metrics() object.

Yes agree. We should do this in its own PR though so that we are sure we have the documentation in place to replace it.

We could, however, have a print() method that simply adds a sentence "the following scores will be computed: 'names of scores'" before printing the list. This could be nice, but maybe not necessary

Yes this is a nice idea and feels like its own issue.

[seabbs]

This looks all good in the main. I have a slight query about the workflow with select_rules and it being internal vs external. See the specific comments.

[seabbs]

this seems like quite a nice workflow for users?

[nikosbosse]

but they can just do rules_point(select = "ape") instead, which is more concise.

I can see a use case once you start combining several lists, e.g. select_rules(c(a(), b()), select = c("c", "d").

[seabbs]

I can see a use case once you start combining several lists,

This would be the argument and the push back on it being more concise is that yes that is true but then each function is doing multiple things which can confuse users. I don't have an extremely strong opinion but think if should be exported.

[nikosbosse]

Latest updates:

• exported select_rules(), but also left the arguments select and exclude in rules_*() in place
• added more explanation to the coverage_90 function in rules_quantile() and changed \(...) to function(...)
• fixed a random typo in the documentation of run_safely()
• changed pkgdown keywords to "metric", although as mentioned previously we need to rethink those at some point
• changed select = all to select = NULL as the default and updated tests accordingly

[seabbs]

LGTM. This seems really slick vs the old version for some reason.

[seabbs]

LGTM. This seems really slick vs the old version for some reason.

[nikosbosse]

Thanks a lot for reviewing!