Skip to content

Conversation

@nikosbosse
Copy link
Collaborator

This PR addresses comments made by review from the Journal of Statistical Software.

It

  • adds documentation for the return value of summarise_scores().
  • removes hard-coded rounding value for correlation(). Previously, the function always rounded correlations to two digits. Instead, this PR introduces a new argument, digits (with the default set to 0, meaning that no rounding takes place).

This is one possible proposal to handle the rounding and there may be other ways to do this. Here are some thoughts:

  • one problem with the current solution is that it breaks user's plots. After the update, their plots will have lots of digits, rather than just two.
  • On the other hand, setting "digits = 2" as the default in order to be consistent with what we have at the moment also seems not ideal. Forcing users to manually set digits = NULL or something like that also feels slightly weird.
  • The digits argument doesn't have to live in correlation(). It could also e.g. live in plot_correlation(). I introduced it here for the following reasons:
    • the output of correlation() is a data.table with numeric and non-numeric columns (the metric names). Rounding them using only data.table code is clunky (I guess you can do something like dt[, lapply(.SD, round, 2), .SDcols = sapply(dt, is.numeric)], but then you still have to manually add back the metric names column).
    • Users might like to have a data.frame with rounded outputs without having to do extra processing
    • if we want to create an S3 method for plot_correlations() at some point, then it seems cleaner to me to have the rounding in correlation() rather than in plot_correlations().

@nikosbosse nikosbosse requested a review from seabbs September 3, 2023 17:57
@nikosbosse nikosbosse changed the base branch from main to scoringutils-review September 3, 2023 17:57
@codecov
Copy link

codecov bot commented Sep 3, 2023

Codecov Report

❗ No coverage uploaded for pull request base (scoringutils-review@e4bc14b). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 4e1d895 differs from pull request most recent head 7a91793. Consider uploading reports for the commit 7a91793 to get more accurate results

@@                  Coverage Diff                   @@
##             scoringutils-review     #320   +/-   ##
======================================================
  Coverage                       ?   90.88%           
======================================================
  Files                          ?       22           
  Lines                          ?     1394           
  Branches                       ?        0           
======================================================
  Hits                           ?     1267           
  Misses                         ?      127           
  Partials                       ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nikosbosse nikosbosse changed the base branch from scoringutils-review to scoringutils-review-backup September 14, 2023 14:03
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.

Looks good. I agree that rounding should likely live in the plotting function if its the default

@nikosbosse nikosbosse merged commit b5360d1 into scoringutils-review Oct 14, 2023
@nikosbosse nikosbosse deleted the scoringutils-review3 branch October 14, 2023 07:39
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