-
Notifications
You must be signed in to change notification settings - Fork 25
simplify plot_pairwise_comparison() #263
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #263 +/- ##
==========================================
- Coverage 91.39% 90.80% -0.59%
==========================================
Files 21 21
Lines 1371 1284 -87
==========================================
- Hits 1253 1166 -87
Misses 118 118
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
need to fix the failing tests first, apologies... |
|
@seabbs Ready now :) |
| plot_pairwise_comparison <- function(comparison_result, | ||
| type = c("mean_scores_ratio", "pval", "together"), | ||
| smaller_is_good = TRUE) { | ||
| type = c("mean_scores_ratio", "pval")) { |
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.
match.arg to check allowed options used.
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.
It does use match.arg in line 782 - is that different from what you meant?
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.
For type? If it does then no that is exactly what I meant
seabbs
left a comment
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.
LGTM to me. I've suggested you use match.arg to check the input for type.
Simplify the function by removing the option to plot both pvalues and mean score ratios together.