Skip to content

Conversation

@nikosbosse
Copy link
Collaborator

Simplify the function by removing the option to plot both pvalues and mean score ratios together.

@nikosbosse nikosbosse requested a review from seabbs January 17, 2023 08:11
@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Merging #263 (91f5adc) into master (55d184c) will decrease coverage by 0.59%.
The diff coverage is 100.00%.

@@            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              
Impacted Files Coverage Δ
R/pairwise-comparisons.R 87.30% <ø> (ø)
R/plot.R 97.52% <100.00%> (-0.44%) ⬇️

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

@nikosbosse
Copy link
Collaborator Author

need to fix the failing tests first, apologies...

@nikosbosse
Copy link
Collaborator Author

@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")) {
Copy link
Contributor

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.

Copy link
Collaborator Author

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?

Copy link
Contributor

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

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.

LGTM to me. I've suggested you use match.arg to check the input for type.

@nikosbosse nikosbosse merged commit 58d93db into master Jan 18, 2023
@nikosbosse nikosbosse deleted the simplify-plot_pairwise_comparison branch January 18, 2023 08:07
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.

2 participants