-
Notifications
You must be signed in to change notification settings - Fork 49
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
Fixes to various row_labels situations with tests. #700
Conversation
Code Coverage Summary
Diff against main
Results for commit: 54fb8e7 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
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.
Thanks @gmbecker. Here is my feedback on some minor things:
- I think it would be better if the analysis variable and analysis function are always annotated regardless of the type of table variants {1-4} as described in [Bug]: row_labels don't match docs in qtable() #698.
If users think the default label is too long it can easily be changed withrow_labels
.
For example:
qtable(ex_adsl , col_vars = "ARM")
Should return
A: Drug X B: Placebo C: Combination
(N=134) (N=134) (N=132)
——————————————————————————————————————————————————————————
STUDYID - count 134 134 132
-
What do you think about changing the default formatting of
avar - afun
label toafun(avar)
. For example, instead ofAGE - max
tomax(AGE)
? Given that the purpose is exploratory, this would be a nice reminder to the person doing the analysis how the table calculations are done. -
This should throw an error as the
row_labels
length shouldn't exceed 1 for table variant {2}:
qtable(ex_adsl , row_vars = "STRATA2", col_vars = "ARM", avar = "AGE", afun = mean,
row_labels = c("ABC", "EFG", "HIJ"))
ABC
EFG A: Drug X B: Placebo C: Combination
HIJ (N=134) (N=134) (N=132)
—————————————————————————————————————————————
S1 34.10 36.46 35.70
S2 33.38 34.40 35.24
- For multi-row returning afun the length of
row_labels
should match the number of cells created byafun
. Right now nothing happens but I think this should also throw an error. Same thing happens in the case of this table with the additionalrow_vars
split:
qtable(ex_adsl, col_vars = "ARM", avar = "AGE", afun = fivenum3,
row_labels = "ABC")
AGE - fivenum3 A: Drug X B: Placebo C: Combination
(N=134) (N=134) (N=132)
————————————————————————————————————————————————————————
21.00 21.00 20.00
28.00 30.00 30.00
33.00 35.00 35.00
39.00 40.00 40.00
50.00 62.00 69.00
@anajens I didn't change the label display but both directions of row_labels - number of afun cells mismatch are now errors. Also there are now separate |
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's working well. Please just add the NEWS update.
Thanks! I think this is a good start but not exactly what I had in mind. I was thinking about being able to return the expression of the layout so users can see directly the connection from qtable to the layout framework. But that's probably a much bigger task for another day ( if users request this). |
That is possible to do but out of scope for this issue, as it would require non-trivial new development. It is a good idea though |
New issue added: #711 |
Close #698.