Repository navigation
Conversation
Unit Tests Summary 1 files 368 suites 2m 0s ⏱️ Results for commit 0ee41c5. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit c7d9106 ♻️ This comment has been updated with latest results. |
…evel, error clearly on non-ARD data frames
…le, regenerate docs
|
Code Coverage SummaryDiff against mainResults for commit: 0ee41c5 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
**What changes are proposed in this pull request?**
* CI and test only, no user-facing change (no NEWS entry).
* `R-CMD-check.yaml`: install XQuartz on macOS. `gdtools` (used by
`flextable`) is linked against X11, which the macOS runners no longer
ship, so every example using flextable failed with "Library not loaded:
/opt/X11/lib/libXrender.1.dylib".
* `test-add_forest.R`: `expect_no_warning()` changed to
`expect_no_error()` when rendering the gt forest plot. R-devel warns
upstream on the out-of-range test values ("object length is not a
multiple of subscript length"); the warning does not come from crane
code.
Both failures are on `main` and block #326.
--------------------------------------------------------------------------------
Pre-review Checklist (if item does not apply, mark is as complete)
- [ ] **All** GitHub Action workflows pass with a ✅
- [x] PR branch has pulled the most recent updates from master branch:
`usethis::pr_merge_main()`
- [x] If a bug was fixed, a unit test was added.
- [x] Code coverage is suitable for any new functions/features
(generally, 100% coverage for new code): `devtools::test_coverage()`
- [ ] Request a reviewer
Co-authored-by: Melkiades <11279768+Melkiades@users.noreply.github.com>
martincadek
left a comment
martincadek
left a comment
There was a problem hiding this comment.
I have run few test on the function on different scenarios and the examples and there are few issues:
- Strata levels swapped with two or more strata
tbl1 <- adsl |>
dplyr::filter(AGEGR1 %in% c("<65", ">80")) |>
tbl_strata(
strata = c(SEX, AGEGR1),
.tbl_fun = ~ tbl_summary(.x, by = ARM, include = AGE),
.combine_with = "tbl_stack",
.combine_args = list(quiet = TRUE)
)
tbl1$df_strata
simplify_ard(tbl1) |>
dplyr::select(cards::all_ard_groups()) |>
cards::unlist_ard_columns() |>
dplyr::distinct()
# got: group2 = AGEGR1 with levels "F", "M"
# group3 = SEX with levels "<65", ">80"
# expected: group2 = AGEGR1 with levels "<65", ">80"
# group3 = SEX with levels "F", "M"
- factor strata becomes character, automatic coecion
adsl_fct <- dplyr::mutate(adsl, SEX = factor(SEX))
tbl2 <- adsl_fct |>
tbl_strata(
strata = SEX,
.tbl_fun = ~ tbl_summary(.x, by = ARM, include = AGE, type = AGE ~ "continuous", statistic = AGE ~ "{mean}"),
.combine_with = "tbl_stack",
.combine_args = list(quiet = TRUE)
)
ard_tbl <- simplify_ard(tbl2) |>
dplyr::filter(context == "summary", !is.na(group1))
ard_qc <- cards::ard_strata(
adsl_fct,
.strata = SEX,
.f = \(df) cards::ard_summary(df, by = ARM, variables = AGE, statistic = ~ cards::continuous_summary_fns("mean"))
)
str(ard_tbl$group2_level[[1]]) # got: chr "F"
str(ard_qc$group2_level[[1]]) # cards: Factor w/ 2 levels "F","M"
- Stratum lost when a factor level contains a comma
adsl_comma <- dplyr::mutate(
adsl,
AGECAT = factor(ifelse(AGE < 70, "Young, <70", "Old, >=70"))
)
tbl3 <- adsl_comma |>
tbl_strata(
strata = AGECAT,
.tbl_fun = ~ tbl_summary(.x, by = ARM, include = AGE),
.combine_with = "tbl_stack",
.combine_args = list(quiet = TRUE)
)
# factor levels are not quoted
names(gather_ard(tbl3))
simplify_ard(tbl3) |>
dplyr::select(cards::all_ard_groups()) |>
cards::unlist_ard_columns() |>
dplyr::distinct()
# got: group2 = tbl_id1 with levels "AGECAT=Old, >=70", "AGECAT=Young, <70"
# expected: group2 = AGECAT with levels "Old, >=70", "Young, <70"
- stat is still a list?
tbl4 <- tbl_summary(adsl_fct, by = ARM, include = c(AGE, SEX))
ard4 <- simplify_ard(tbl4, .unlist = TRUE)
class(ard4$stat)
I would also suggest to consider naming group and group_level as strata and strata_level unless that collides with anything.
`df_strata` keeps the strata columns in the order of the data, so with `strata` in another order the levels went to the wrong variable. The levels now come from `df_strata` by name and keep their type (factor, numeric). The variable names are split from the names of the pieces with the levels known, so factor levels with `,` or `=` are read right. The strata of one call are numbered in the order of `strata`, as `cards::ard_strata()` does, and those of nested calls come first, as with nested `ard_strata()` calls. `.unlist` is removed: `stat` cannot be unlisted while the attributes rows are in, so filter the statistics, then use `cards::unlist_ard_columns()`. The `compare_ard()` snippet in the details uses the names of the example.
|
Thanks for the careful checks. All four reproduce: three were real bugs, and the fourth showed that 1) Strata levels swapped. and 2) Factor strata become character. Right, and it made str(ard_tbl$group2_level[[1]])
#> Factor w/ 2 levels "F","M": 1
3) Stratum lost with a comma in a factor level. gtsummary quotes character levels in the names of the pieces, but not factor levels, so This also fixes a quieter variant: levels like 4) simplify_ard(tbl4) |>
dplyr::filter(context != "attributes") |>
cards::unlist_ard_columns()gives a numeric
|
|
Thanks @martincadek!! very good catches! let me know what you think of last update |
What changes are proposed in this pull request?
simplify_ard()to flatten the ARD of a table, including stratified, stacked and split tables, into one ARD with the strata as group columns, ready forcards::compare_ard(). (Addsimplify_ard()to flatten the ARD of stratified, stacked and split tables #325)Statistics are identified by groups, variables,
contextandstat_name.contextis part of the key because some tables report two statistics with the same name for the same rows (subject and event counts intbl_hierarchical_rate_and_count()), socompare_ard()needs the same keys:The split duplication itself is a gtsummary issue (
tbl_split_by_rows()keeps the full$cardsand$tblsin every piece);simplify_ard()works around it until that is fixed upstream.Reference GitHub issue associated with pull request. closes #325
Pre-review Checklist (if item does not apply, mark is as complete)
usethis::pr_merge_main()devtools::test_coverage()