122 factors best response aggregate recist rates#123
Conversation
DanChaltiel
left a comment
DanChaltiel
left a comment
There was a problem hiding this comment.
J'ai relu, c'est nickel 👍
Juste quelques commentaires mineurs.
Bien vu l'histoire de l'ordre des footnotes, il faudra peut-être brainstormer ça en réunion grstat (et on garde ça en tête pour #111, à voir si des lettres ne seraient pas plus lisibles que des astérisques)
Par contre je ne suis pas sûr de comprendre le problème initial (factor pour avoir tous les niveaux, dans l'ordre), même sans tes modifs le test passait bien :
C'est quand on modifie
best_response (genre as.character) avant de passer aggregate() ?
Si c'est ça, tu pourrais ajouter un test à cet endroit ?
Il faut un cas qui plantait avec ton code d'avant et que ta PR résout.
| levels = best_response_label)) | ||
|
|
||
| if(length(recist$subjid) != length(data$subjid)){ | ||
| if(length(recist$subjid) != length(data$subjid) | length(unique(recist$subjid)) != length(recist$subjid)){ |
There was a problem hiding this comment.
Ton code marchera certainement, mais la logique de condition est un peu bancale :
length(recist$subjid)c'est en fait simplementnrow(recist), peu importe la colonne sélectionnée- Comme tu appelles
distinct()surdatapour avoirrecist, comparerdataetrecistrevient à tester quedataest distinct, non ? Mais du coup distinct sur toutes les colonnes, pas justesubjid.
Est-ce qu'on ne peut pas simplifier ça en un check sur data (avant de créer recist) ?
Par exemple juste : if(anyDuplicated(data$subjid)) cli_abort(...)
| recist = data %>% | ||
| distinct() | ||
| distinct() %>% | ||
| mutate(six_months_confirmation = as.logical(six_months_confirmation), |
There was a problem hiding this comment.
Vu que tu ajoutes des trucs, je reviewe des trucs 😁
Je n'avais pas vu mais il n'y a aucune programmation défensive ici
Tu pourrais ajouter un check des colonnes :
assert_names_exists(data, c("best_response", "six_months_confirmation", etc))
|
|
||
| data_br_3 = data_br %>% | ||
| mutate(best_response = ifelse(subjid ==1, "Stable disease", as.character(best_response))) | ||
| aggregate_recist_rates(data_br_3) |
There was a problem hiding this comment.
Nickel le test !
Par contre il faudrait retirer les lignes avec juste aggregate_recist_rates(data_br_d) (ligne 105 aussi)
Ça prend du temps machine pendant les tests juste pour vérifier que la ligne ne donne pas d'erreur, alors qu'on teste déjà ça dans le snapshot juste au-dessous.
| }) | ||
| }) | ||
|
|
||
| test_that("No bug when modification of best_response before between calc_best_resp and aggregatte", { |
2 participants
No description provided.