-
Notifications
You must be signed in to change notification settings - Fork 21
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
Rename pairwise comparisons #722
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #722 +/- ##
=======================================
Coverage 95.27% 95.27%
=======================================
Files 21 21
Lines 1630 1630
=======================================
Hits 1553 1553
Misses 77 77 ☔ View full report in Codecov by Sentry. |
For this little one shall we just chuck in an alias? It seems a bit cruel! Or shall we just be cruel? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all seems good to me. See comment about the cruelty of the s but there are so many breaking changes people will already be on the alert?
Maybe we should just be cruel? I don't know, the downside to aliases that I see is that they clutter the docs and the autocomplete dropdown |
Dear future user, if you're reading this and are upset: we apologise for our cruelty. Please blame it on me and only me. |
Description
As discussed in #638, this PR
add_pairwise_comparison()
toadd_relative_skill()
pairwise_comparison()
toget_pairwise_comparisons()
plot_pairwise_comparison()
toplot_pairwise_comparisons()
which is a breaking change I'm sure people will love...The rationale for this change is this:
add_relative_skil()
is a bit more descriptive thanadd_pairwise_comparison()
in terms of what it doesget_pairwise_comparisons()
would make it clear that these are two different workflows with two different purposes (one mainly for plotting, the other for computing scores).get_pairwise_comparisons()
would make the naming of the function more in line with the otherget_
-functionsget_pairwise_comparisons()
rather thanget_relative_skill()
because the main thing this function is meant to give you according to our updated workflow is the (visualisation of) mean score ratios (see below)plot_pairwise_comparison()
toplot_pairwise_comparisons()
is a bit stupid. Butget_pairwise_comparisons()
feels more appropriate thanget_pairwise_comparison()
and theget_
and theplot_
function should be consistent. We could do aliases? Or stick with the singular after all.Visualisation of mean score ratios:
Checklist
lintr::lint_package()
to check for style issues introduced by my changes.