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 #383 - Remove available_metrics() #706

Merged
merged 8 commits into from
Mar 9, 2024
Merged

Conversation

nikosbosse
Copy link
Contributor

@nikosbosse nikosbosse commented Mar 8, 2024

Description

This PR closes #383

available_metrics() is a remnant of the sunnier days when scoringutils had a restricted list of known metrics. Now that users can specify their own metrics, the function is not needed anymore and should be removed.

This PR

  • removes available_metrics()
  • to do this plot_correlations() needed to be updated which relied on it. avaiable_metrics() is now changed to get_score_names()
  • for that to work, the input to plot_correlations() has to have a score_names attribute. The PR therefore updates correlation() to output a scores object with the necessary score_names attribute
  • since the original correlation() outputs a data.table with rownames, new_scores() was updated to now have a ... argument which is used within correlation to specify keep.rownames = TRUE. Within correlation there is now an explicit copy statement, because without that we're getting an internal self reference error (data.table isn't happy about the rownames...). I feel like this is sufficient to address the issue here and always making a copy at the end of new_scores() might be an overkill
  • updates a few tests
  • removes available_metrics() from get_protected_columns()

The data.table copy issue should maybe be an issue for data.table...
Reprex:

x <- data.frame(x = 1:10, row.names = paste0("test_", 1:10))
x <- as.data.table(x, keep.rownames = TRUE)
class(x) <- c("test", class(x))
x[, testtest := 1]

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 requested a review from seabbs March 8, 2024 19:28
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.28%. Comparing base (a45f4f5) to head (8a18e16).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #706      +/-   ##
==========================================
- Coverage   95.28%   95.28%   -0.01%     
==========================================
  Files          21       21              
  Lines        1634     1632       -2     
==========================================
- Hits         1557     1555       -2     
  Misses         77       77              

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

@seabbs seabbs enabled auto-merge (rebase) March 9, 2024 12:35
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.

All seems sensible to me. Agree on copy.

@nikosbosse nikosbosse disabled auto-merge March 9, 2024 18:28
@nikosbosse nikosbosse merged commit 5db9070 into main Mar 9, 2024
11 checks passed
@nikosbosse nikosbosse deleted the remove-available_metrics branch March 9, 2024 18:28
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.

Get rid of available_metrics()
2 participants