Conversation
gestion des NA dans target_sum_diff_first + création de test-waterfall-plot.R
DanChaltiel
left a comment
DanChaltiel
left a comment
There was a problem hiding this comment.
Super, merci Anne !
Voici mes commentaires,
En fait il s'agit surtout d'un bug de forcats.
Je t'ai mis le workaround, ça t'évitera bien des soucis.
Même en corrigeant le bug, on a un warning ggplot donc ça vaut toujours le coup de mettre un warning nous-mêmes pour être plus clairs et pour un plot plus joli.
There was a problem hiding this comment.
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 :-)
| expect_warning( | ||
| p <- waterfall_plot(data_na, warnings = TRUE), | ||
| regexp = "ignore" | ||
| ) |
There was a problem hiding this comment.
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"
| target_sum_diff_first = runif(n(), -0.6, 0.3), | ||
| target_sum_diff_first = replace(target_sum_diff_first, c(1, 3, 10), NA) |
There was a problem hiding this comment.
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| 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 . | ||
| } %>% |
There was a problem hiding this comment.
- 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 ?
2 participants
gestion des NA dans target_sum_diff_first + création de test-waterfall-plot.R