-
-
Notifications
You must be signed in to change notification settings - Fork 15
Add module for summary tables with gt #956
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
Conversation
Unit Tests Summary 1 files 24 suites 15m 57s ⏱️ For more details on these failures, see this check. Results for commit 920b107. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit a4d9f3c ♻️ This comment has been updated with latest results. |
Code Coverage SummaryDiff against mainResults for commit: 42080fe Minimum allowed coverage is ♻️ This comment has been updated with latest results |
|
|
||
| # sort | ||
| if (!is.null(sort)) { | ||
| validate(need(all(vapply(sort, is, class2 = "formula", logical(1L))), "All elements of sort should be formulas")) |
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.
| validate(need(all(vapply(sort, is, class2 = "formula", logical(1L))), "All elements of sort should be formulas")) | |
| validate(need(all(vapply(list(sort), is, class2 = "formula", logical(1L))), "All elements of sort should be formulas")) |
sort has to be list since we're running it against vapply.
Otherwise, I got the validation:
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.
Did you use sort = all_categorical(FALSE) ~ "alphanumeric"? I should change a bit the validation to account for that case. The proposed change I think wouldn't work for sort = list(all_categorical(FALSE) ~ "alphanumeric", all_numeric(TRUE) ~ "alphanumeric") (pseudocode to show how I thought this parameter would be used).
| missing_text = "<Missing>", | ||
| missing_stat = "{N_nonmiss}", | ||
| sort = gtsummary::all_categorical(FALSE) ~ "alphanumeric", | ||
| include = tidyselect::everything(), |
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.
This won't work. I'm sceptical if tidyselect::everything can be used with data_extract_spec. tidyselect::everything() fails when forced (executed outside of tidyselect::eval_tidy). Referring to the comment I think that if picks succeeds and will be implemented this will have to be deprecated and changed to picks.
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.
Yes, the default arguments won't work. I just wanted to have here to make it easier for users to match the arguments of the gtsummary function to the module ones. I'll change it.
My comment about picks is now what is accepted and what is not accepted but about what is the R code shown the users. If a users use: picks(variables(where(is.numeric))) but we evaluate that and replace by the name of the variables ("AGE", ...) , the "Show R code" won't show that the app developer set the selection to the numeric values. It will only show the variables selected: include = where(is.numeric) vs include = c(AGE, ...). There is some information lost there that could be relevant.
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.
Yes, unfortunately original parameter will be lost. I don't think we can preserve tidyselect specs to use them in the final visualization.
Let's explore this more with picks when it will be reviewed more seriously 👍
| ) { | ||
| message("Initializing tm_gt_tbl_summary") | ||
| checkmate::assert_string(label) | ||
| if (inherits(by, "data_extract_spec")) { |
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.
If you want to use tidyselect and assert on this argument, then you need to handle this in a specific way. See here:
https://github.com/insightsengineering/teal.transform/blob/621603df655f4c051de612f1c35a66cf44a81756/R/0-picks.R#L427
|
Closing this PR as development has moved elsewhere. |
Pull Request
Fixes https://github.com/insightsengineering/coredev-tasks/issues/676
See also https://github.com/insightsengineering/coredev-tasks/issues/658
Add a new module using crane -> gtsummary->gt to summarise tables.
TODO:
Other input should be defined by the app-developer.
add_*functions on the module.Example app
I haven't added more UI parameters are they are complex, a mix of list, formulas and characters that are not easily translated to the UI (without a free text).
For a full implementation these tasks would need to be taken: