Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions R/waterfall_plot.R
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#' @importFrom ggplot2 aes facet_wrap geom_col geom_hline ggplot labs scale_fill_manual scale_x_discrete scale_y_continuous theme_minimal
#' @importFrom rlang check_dots_empty
#' @importFrom scales breaks_width label_percent
#' @importFrom cli cli_warn
#'
#' @examples
#' db = grstat_example(N=50)
Expand Down Expand Up @@ -76,6 +77,14 @@ waterfall_plot = function(data, ...,

db_wf = data %>%
rename(shape=any_of2(shape), resp=all_of(fill), y=all_of(y)) %>%
{
na_rows = which(is.na(.$y))
na_y = length(na_rows)
if(na_y > 0){
if(warnings) cli_warn("{.fun waterfall_plot} will ignore {na_y} observation{?s} with missing {.var {y}} and subjid : {.val { . $subjid[na_rows]}}.")
filter(., !is.na(y))
} else .
} %>%
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • On essaie d'éviter les {} après un pipe, c'est considéré comme peu lisible : on est censé être "à l'intérieur" d'une table mais on a des if/else au liste de ifelse(), ça oblige à utiliser . partout, etc.
  • C'est en fait un bug de forcats, qu'on peut résoudre avec fct_reorder2(.na_rm=FALSE)
  • Sur l'objet qui en résulte (sans erreur), tu pourras facilement checker les NA et émettre ton warning
  • Ajoute une classe à ton warning (cf. mon commentaire dans les tests).
  • Est-ce qu'on veut vraiment enlever les NA de toutes les colonnes ? Par exemple, ça ne me choquerait pas d'avoir une réponse manquante, ça ajouterait juste une classe grise dans les couleurs.
  • Par contre si on ajoute des NA dans la réponse, l'ordre devient bizarre, on fait une autre issue ou tu veux essayer de réparer ?

mutate(subjid = forcats::fct_reorder2(as.character(subjid),
as.numeric(resp), y))

Expand Down
35 changes: 0 additions & 35 deletions tests/testthat/test-best_response.R
Original file line number Diff line number Diff line change
Expand Up @@ -11,39 +11,4 @@ test_that("calc_best_response", {
})


test_that("waterfall_plot", {
local_options(grstat_lifecycle_verbosity="quiet")

db = grstat_example(N=50)
data_best_resp = calc_best_response(db$recist)

#simple example
p1 = waterfall_plot(data_best_resp)
vdiffr::expect_doppelganger("waterfall-plot-default", p1)

#facet by arm
p2 = data_best_resp %>%
left_join(db$enrolres, by="subjid") %>%
waterfall_plot(arm="ARM")
vdiffr::expect_doppelganger("waterfall-plot-arms", p2)


#add symbols
#use the NA level to not show the case
set.seed(0)
data_symbols = db$recist %>%
summarise(new_lesion=ifelse(any(rcnew=="Yes", na.rm=TRUE), "New lesion", NA),
random=cut(runif(1), breaks=c(0,0.05,0.1,1), labels=c("A", "B", NA)),
.by=subjid)

p3 = data_best_resp %>%
left_join(data_symbols, by="subjid") %>%
waterfall_plot(shape="new_lesion")
vdiffr::expect_doppelganger("waterfall-plot-shape1", p3)

p4 = data_best_resp %>%
left_join(data_symbols, by="subjid") %>%
waterfall_plot(shape="random") +
labs(shape="Event")
vdiffr::expect_doppelganger("waterfall-plot-shape2", p4)
})
51 changes: 51 additions & 0 deletions tests/testthat/test-waterfall-plot.R
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nickel le nouveau fichier.
Essaie de séparer en plusieurs commits si tu peux : 1 commit ou tu bouges le code, et un commit ou tu ajoutes ta partie. Ca simplifie la review :-)

Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
test_that("waterfall_plot", {
local_options(grstat_lifecycle_verbosity="quiet")

db = grstat_example(N=50)
data_best_resp = calc_best_response(db$recist)

#simple example
p1 = waterfall_plot(data_best_resp)
vdiffr::expect_doppelganger("waterfall-plot-default", p1)

#facet by arm
p2 = data_best_resp %>%
left_join(db$enrolres, by="subjid") %>%
waterfall_plot(arm="ARM")
vdiffr::expect_doppelganger("waterfall-plot-arms", p2)


#add symbols
#use the NA level to not show the case
set.seed(0)
data_symbols = db$recist %>%
summarise(new_lesion=ifelse(any(rcnew=="Yes", na.rm=TRUE), "New lesion", NA),
random=cut(runif(1), breaks=c(0,0.05,0.1,1), labels=c("A", "B", NA)),
.by=subjid)

p3 = data_best_resp %>%
left_join(data_symbols, by="subjid") %>%
waterfall_plot(shape="new_lesion")
vdiffr::expect_doppelganger("waterfall-plot-shape1", p3)

p4 = data_best_resp %>%
left_join(data_symbols, by="subjid") %>%
waterfall_plot(shape="random") +
labs(shape="Event")
vdiffr::expect_doppelganger("waterfall-plot-shape2", p4)

set.seed(123)
data_na =
data_best_resp %>%
dplyr::mutate(
target_sum_diff_first = runif(n(), -0.6, 0.3),
target_sum_diff_first = replace(target_sum_diff_first, c(1, 3, 10), NA)
Comment on lines +41 to +42
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternativement, tu peux :

  • mettre NA dans x% des cas :
target_sum_diff_first = if_else(runif(n())<0.2, NA, target_sum_diff_first) # 20% de NA
  • mettre NA pour les x-ièmes lignes :
target_sum_diff_first = if_else(row_number()>40, NA, target_sum_diff_first), # 10 dernières
best_response = if_else(row_number()<10, NA, best_response), # 10 premières

)

expect_warning(
p <- waterfall_plot(data_na, warnings = TRUE),
regexp = "ignore"
)
Comment on lines +45 to +48
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarde mes appels à expect_warning() dans les autres fichiers : tu appelles la fonction et tu pipe ton expect.
En utilisant une classe plutôt qu'un regexp, ça nous laisse libre de changer le message sans avoir à changer le test, et ça évite une interaction avec un autre warning émis par une fonction interne et qui contiendrait "ignore"



})
Loading