Support display of higher-level ns #876

gmbecker · 2024-06-04T22:02:07Z

closes #135 and #752

github-actions · 2024-06-04T22:16:41Z

Unit Tests Summary

1 files 25 suites 1m 37s ⏱️
209 tests 209 ✅ 0 💤 0 ❌
1 560 runs 1 560 ✅ 0 💤 0 ❌

Results for commit 8ba5023.

♻️ This comment has been updated with latest results.

github-actions · 2024-06-04T22:17:20Z

Unit Test Performance Difference

Test Suite	$Status$	Time on `main`	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
Printing tables	💔	$4.08$	$+1.82$	$+18$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on `main`	$±Time$	Test Case
Printing tables	👶		$+2.03$	showing_higher_level_ncols_works
Tabulation framework	💚	$9.98$	$-1.01$	qtable_works
binding	👶		$+0.56$	count_visibility_syncing_works_when_cbinding
regression tests	👶		$+0.18$	overridden_colcounts_via_build_table_are_used_during_tabulation_correctly

Results for commit be0cb8c

♻️ This comment has been updated with latest results.

github-actions · 2024-06-04T22:19:11Z

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/00tabletrees.R               780      63  91.92%   20, 94, 97, 428, 519-520, 523, 681, 785, 877-878, 980, 983, 985-986, 1004-1007, 1027, 1142-1145, 1243-1248, 1404, 1504-1507, 1573-1576, 1612-1615, 1621-1626, 1677, 1684, 1778, 1886, 1899, 1902-1905, 1908-1911, 1938, 1969-1970
R/as_html.R                    167      25  85.03%   5-10, 74, 146-151, 156-161, 176-180, 267
R/colby_constructors.R         594      24  95.96%   81, 134, 197-200, 267-270, 410, 426, 1175, 1263, 1424, 1463, 1485, 1509, 1530, 1676, 1899-1902
R/compare_rtables.R             83      17  79.52%   93-96, 99-102, 115-118, 137, 156-157, 188, 193
R/format_rcell.R                12       0  100.00%
R/indent.R                      13       2  84.62%   40-41
R/index_footnotes.R             66       0  100.00%
R/make_split_fun.R             138      31  77.54%   22-26, 36-39, 52-55, 58-61, 115, 119, 267, 270-273, 278-281, 295, 366, 375, 377, 379, 430
R/make_subset_expr.R           137      15  89.05%   35, 47-61, 135-142, 178, 267, 271, 280
R/simple_analysis.R              5       1  80.00%   56
R/split_funs.R                 510      66  87.06%   127, 132, 138-143, 156, 173-177, 353-358, 375-380, 456, 502, 518-521, 537, 599, 609-610, 612, 624, 668, 693, 868, 875, 899-902, 913-914, 916, 918, 1090-1092, 1106-1110, 1174-1177, 1240-1243
R/summary.R                    144      38  73.61%   35, 80, 178-220, 269, 315-331, 366, 397
R/tree_accessors.R            1120     140  87.50%   113, 142-143, 267, 287, 313, 336, 366, 384, 403-407, 429, 451-454, 569, 596-597, 883-889, 1033, 1052, 1078, 1130, 1187-1188, 1225, 1260, 1298-1303, 1362, 1436-1440, 1458-1467, 1545, 1660-1663, 1688, 1710-1711, 1721, 1772, 1793-1798, 1819-1824, 1835, 1909, 1950, 2049, 2156, 2169, 2183, 2199, 2208, 2218-2222, 2272-2277, 2480, 2490-2493, 2503, 2528-2531, 2538, 2540-2543, 2665, 2699-2700, 2757, 3062, 3423, 3539, 3573-3598, 3689-3697, 3850, 3924-3930, 4142-4143, 4150, 4153-4156, 4160, 4210, 4271, 4296-4320
R/tt_afun_utils.R              411      32  92.21%   48, 155, 162, 171-184, 250, 258-259, 477, 485-488, 570-574, 594, 608-610
R/tt_compare_tables.R           70       4  94.29%   51, 174, 246, 250
R/tt_compatibility.R           570      62  89.12%   19, 142-143, 186, 191, 319-320, 324-327, 333, 337, 521, 575-578, 615-617, 655, 688, 708, 741-744, 789, 806-810, 893, 920-923, 932, 994, 1002, 1013-1016, 1127, 1134, 1162-1176, 1207-1208
R/tt_dotabulation.R           1161      95  91.82%   54, 246, 251, 253, 301, 325, 329-332, 364-367, 390, 423-426, 454-457, 552, 690-694, 743, 747, 775-778, 788, 808-812, 819-822, 1082, 1086, 1117, 1220-1223, 1433-1441, 1705-1714, 1796-1799, 1810, 1815, 1820-1821, 1823, 1834, 1839, 1862, 1948-1967
R/tt_export.R                  513      31  93.96%   43, 181-185, 233-236, 288-291, 436, 442, 474, 526, 806, 815, 840-844, 1011-1014, 1017, 1048, 1054
R/tt_from_df.R                  15       0  100.00%
R/tt_paginate.R                513      37  92.79%   74, 122-131, 441, 576-579, 600-604, 749-752, 803-810, 887, 890, 908, 915, 918
R/tt_pos_and_access.R          571      42  92.64%   76, 78-80, 105, 166, 212-216, 258, 507, 509, 517, 523, 537, 547-550, 730, 733, 741-745, 750-753, 780, 833-834, 845, 1007-1008, 1076-1092, 1361, 1436
R/tt_showmethods.R             162      21  87.04%   56, 91-113, 223, 249, 258, 263, 266-270, 359-360
R/tt_sort.R                    101       5  95.05%   245-248, 256
R/tt_toString.R                436      27  93.81%   123, 345, 365-368, 374, 387, 397, 403, 406, 412-422, 515, 616, 818-843
R/utils.R                       29       0  100.00%
R/validate_table_struct.R       84      10  88.10%   80-84, 93-94, 140, 149-150
R/Viewer.R                      61       9  85.25%   46, 50, 60-64, 84, 118
TOTAL                         8466     797  90.59%

Diff against main

Filename                  Stmts    Miss  Cover
----------------------  -------  ------  -------
R/00tabletrees.R            +29      +1  +0.18%
R/colby_constructors.R      +27      +4  -0.51%
R/make_subset_expr.R         -6      -1  +0.24%
R/tree_accessors.R         +172     +37  -1.64%
R/tt_compatibility.R        +50      -1  +1.24%
R/tt_dotabulation.R         +36      -1  +0.35%
R/tt_paginate.R             +14      -1  +0.40%
R/tt_pos_and_access.R         0      -1  +0.18%
R/tt_showmethods.R          +18       0  +1.62%
R/tt_toString.R             +40       0  +0.63%
TOTAL                      +380     +37  -0.02%

Results for commit: 8ba5023

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

…rtables into 135_higher_level_ns

gmbecker · 2024-06-04T22:37:00Z

I hate you spellchecker

R/tt_showmethods.R

R/tt_dotabulation.R

inst/WORDLIST

tests/testthat/test-printing.R

.pre-commit-config.yaml

R/make_subset_expr.R

Signed-off-by: Davide Garolini <[email protected]>

NEWS.md

gmbecker · 2024-06-11T17:06:17Z

Lgtm! Thanks Gabe for this update. I am wondering only if this is doing nothing on purpose:

lyt2 <- basic_table() %>%
  split_cols_by("ARM") %>%
  split_cols_by("SEX",
    split_fun = keep_split_levels(c("F", "M")),
    show_colcounts = TRUE
  ) %>%
  analyze("AGE")

tbl2 <- build_table(lyt2, ex_adsl)
tbl2
colcount_format(tbl2) <- "xx"
tbl2 # the same

So the issue here is with what that should do.

I think the most reasonable thing for that to do would be to reset all colcount formats throughout the column tree. The issue with that is that then colcount_format<- isn't really the setter that corresponds with colcount_format the getter anymore. They would have the same name but only somewhat corresponding behaviors.

Its probably still the right thing, but what are your thoughts? @Melkiades

Melkiades · 2024-06-11T17:47:50Z

Lgtm! Thanks Gabe for this update. I am wondering only if this is doing nothing on purpose:
lyt2 <- basic_table() %>%
  split_cols_by("ARM") %>%
  split_cols_by("SEX",
    split_fun = keep_split_levels(c("F", "M")),
    show_colcounts = TRUE
  ) %>%
  analyze("AGE")

tbl2 <- build_table(lyt2, ex_adsl)
tbl2
colcount_format(tbl2) <- "xx"
tbl2 # the same
So the issue here is with what that should do.

I think the most reasonable thing for that to do would be to reset all colcount formats throughout the column tree. The issue with that is that then colcount_format<- isn't really the setter that corresponds with colcount_format the getter anymore. They would have the same name but only somewhat corresponding behaviors.

Its probably still the right thing, but what are your thoughts? @Melkiades

To my mind, colcount_format<- and colcount_visibility<- should be able to set all to the same value when path = NULL or specific values when path is specified. Similarly, I understand why you added facet_colcounts<- and facet_colcounts_visibility<- but it is difficult to motivate to a generic user why you need another function to do a very similar thing. Personally, I would cover the 3 relevant things: formats, visibility, and values, with only one function each (in that case, for a facet is enough to use the node in the path)

gmbecker · 2024-06-11T19:00:10Z

Lgtm! Thanks Gabe for this update. I am wondering only if this is doing nothing on purpose:
lyt2 <- basic_table() %>%
  split_cols_by("ARM") %>%
  split_cols_by("SEX",
    split_fun = keep_split_levels(c("F", "M")),
    show_colcounts = TRUE
  ) %>%
  analyze("AGE")

tbl2 <- build_table(lyt2, ex_adsl)
tbl2
colcount_format(tbl2) <- "xx"
tbl2 # the same
So the issue here is with what that should do.
I think the most reasonable thing for that to do would be to reset all colcount formats throughout the column tree. The issue with that is that then colcount_format<- isn't really the setter that corresponds with colcount_format the getter anymore. They would have the same name but only somewhat corresponding behaviors.
Its probably still the right thing, but what are your thoughts? @Melkiades
To my mind, colcount_format<- and colcount_visibility<- should be able to set all to the same value when path = NULL or specific values when path is specified. Similarly, I understand why you added facet_colcounts<- and facet_colcounts_visibility<- but it is difficult to motivate to a generic user why you need another function to do a very similar thing. Personally, I would cover the 3 relevant things: formats, visibility, and values, with only one function each (in that case, for a facet is enough to use the node in the path)

There are a few things that I think make that a non-starter, though I agree it would be nice in principle.

First, we cannot do that for values, because assigning a vector of values onto the nodes of a tree structure isn't trivial or something the user is going to be able to easily understand.

In other words, if we have

> lyt <- basic_table() %>% split_cols_by("ARM") %>% split_cols_by("SEX") %>% analyze("AGE")
> tbl <- build_table(lyt, ex_adsl)
> tbl
                      A: Drug X                                  B: Placebo                               C: Combination             
         F       M       U     UNDIFFERENTIATED     F       M       U     UNDIFFERENTIATED     F       M       U     UNDIFFERENTIATED
—————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————
Mean   32.76   35.57   31.67        28.00         34.12   37.44   31.00          NA          35.20   35.38   35.25        45.00

And we say

facet_colcounts(tbl, NULL) <- 1:15

where are each of those values going to be assigned? A: Drug X will likely get a 1 under it, but does the 2 go under B: Placebo" or under the FunderA: Drug X`? Its not clear to me and it would be extremely unclear to the user, I think.

Secondly, for backwards compatability we need col_counts<- with no path specification to only modify the leaf nodes. We might be able to hack that behavior in by differentiating between NULL and NA, but it would be ugly and complicate the code and the above issue would remain anyway

For visibility and format, the issue is different desired behavior. I am not convinced turning on visibility of all column counts for tables with non-trivial column structure would ever/even remotely often be what the user wants to do, they're likely to want to turn on and off specific (sets of) column counts. For format, though, the opposite is true, if they are ever in a position to want to modify the colcount format for an already built table, they very likely do want it to be changed across the board, and are quite unlikely to only want to change the format of specific column counts.

Melkiades

@gmbecker {scda.test} fails after this. It is because before, adding col_counts to build_table() was forcing to print the col counts, now it is no more true. How do you want to proceed? shall we reinstate that?

…isplay for leaf cols

Signed-off-by: Gabe Becker <[email protected]>

gmbecker · 2024-06-12T19:08:11Z

@gmbecker {scda.test} fails after this. It is because before, adding col_counts to build_table() was forcing to print the col counts, now it is no more true. How do you want to proceed? shall we reinstate that?

fixed @Melkiades, with documentation (behavior wasn't documented before) and a regression test, though that argument is marked as "soft deprecated" in the documentation (and not just by me, since it uses lifecycle which I dont), so official templates arguably "shouldn't" be relying on that feature

inst/WORDLIST

wordlist update Signed-off-by: Joe Zhu <[email protected]>

shajoezhu · 2024-06-13T03:06:34Z

I have read the CLA Document and I hereby sign the CLA

Melkiades · 2024-06-13T14:40:01Z

@gmbecker There are still changes in snapshots downstream for aet04_pi

Melkiades · 2024-06-13T15:08:10Z

For example:

── Failed tests ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Failure (test-table_aet04_pi.R:66:3): AET04_PI full table is produced correctly
Snapshot of code has changed:
old[6:31] vs new[6:31]
                                                                  (N=86)          (N=86)         (N=86)         (N=72)          (N=72)         (N=72)         (N=96)          (N=96)         (N=96)
    ——————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————
    SKIN AND SUBCUTANEOUS TISSUE DISORDERS
-   Total number of patients with at least one adverse event    19 (22.1%)       4 (4.7%)           0         34 (47.2%)       6 (8.3%)           0         35 (36.5%)       6 (6.2%)       2 (2.1%)
+   Total number of patients with at least one adverse event    19 (35.2%)       4 (44.4%)          0         34 (27.6%)       6 (40.0%)          0         35 (30.7%)       6 (40.0%)      2 (100%)
-     PRURITUS                                                       9               3              0             36               8              0             24               1              1
+     PRURITUS                                                      15              33              0             21              40              0             20               7             50
-     ERYTHEMA                                                      10               0              0             19               0              0             16               2              0
+     ERYTHEMA                                                      17               0              0             11               0              0             13              13              0
-     RASH                                                           6               1              0             15               1              0             14               2              0
+     RASH                                                           9              11              0              9               7              0             11              13              0
-     SKIN IRRITATION                                                3               0              0              7               0              0              6               1              1
+     SKIN IRRITATION                                                6               0              0              4               0              0              5               7             50
    GENERAL DISORDERS AND ADMINISTRATION SITE CONDITIONS
-   Total number of patients with at least one adverse event    10 (11.6%)       2 (2.3%)           0         29 (40.3%)       5 (6.9%)           0         31 (32.3%)       8 (8.3%)           0
+   Total number of patients with at least one adverse event    10 (18.5%)       2 (22.2%)          0         29 (23.6%)       5 (33.3%)          0         31 (27.2%)       8 (53.3%)          0
and 16 more ...

* Run `testthat::snapshot_accept('table_aet04_pi')` to accept the change.
* Run `testthat::snapshot_review('table_aet04_pi')` to interactively review the change.

Failure (test-table_aet04_pi.R:88:3): AET04_PI variant 1 is produced correctly

shajoezhu · 2024-06-13T15:11:07Z

this doesnt look right

shajoezhu · 2024-06-13T15:39:25Z

downstream was broken in https://github.com/insightsengineering/scda.test/actions/runs/9502364605/job/26190158465?pr=141

Melkiades · 2024-06-13T16:01:08Z

this doesnt look right

Now the non-cran works so it these are coming out. I had the issue on my local. No issues from main, table_aet04_pi is broken from this PR

…rgs correctly

gmbecker · 2024-06-14T18:53:38Z

For example:

── Failed tests ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Failure (test-table_aet04_pi.R:66:3): AET04_PI full table is produced correctly
Snapshot of code has changed:
old[6:31] vs new[6:31]
                                                                  (N=86)          (N=86)         (N=86)         (N=72)          (N=72)         (N=72)         (N=96)          (N=96)         (N=96)
    ——————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————
    SKIN AND SUBCUTANEOUS TISSUE DISORDERS
-   Total number of patients with at least one adverse event    19 (22.1%)       4 (4.7%)           0         34 (47.2%)       6 (8.3%)           0         35 (36.5%)       6 (6.2%)       2 (2.1%)
+   Total number of patients with at least one adverse event    19 (35.2%)       4 (44.4%)          0         34 (27.6%)       6 (40.0%)          0         35 (30.7%)       6 (40.0%)      2 (100%)
-     PRURITUS                                                       9               3              0             36               8              0             24               1              1
+     PRURITUS                                                      15              33              0             21              40              0             20               7             50
-     ERYTHEMA                                                      10               0              0             19               0              0             16               2              0
+     ERYTHEMA                                                      17               0              0             11               0              0             13              13              0
-     RASH                                                           6               1              0             15               1              0             14               2              0
+     RASH                                                           9              11              0              9               7              0             11              13              0
-     SKIN IRRITATION                                                3               0              0              7               0              0              6               1              1
+     SKIN IRRITATION                                                6               0              0              4               0              0              5               7             50
    GENERAL DISORDERS AND ADMINISTRATION SITE CONDITIONS
-   Total number of patients with at least one adverse event    10 (11.6%)       2 (2.3%)           0         29 (40.3%)       5 (6.9%)           0         31 (32.3%)       8 (8.3%)           0
+   Total number of patients with at least one adverse event    10 (18.5%)       2 (22.2%)          0         29 (23.6%)       5 (33.3%)          0         31 (27.2%)       8 (53.3%)          0
and 16 more ...

* Run `testthat::snapshot_accept('table_aet04_pi')` to accept the change.
* Run `testthat::snapshot_review('table_aet04_pi')` to interactively review the change.

Failure (test-table_aet04_pi.R:88:3): AET04_PI variant 1 is produced correctly

Should be fixed, I think, with regression test @Melkiades @shajoezhu

shajoezhu

lgtm! Thanks @gmbecker

ready to move forward, downstream breaking change fixed, will revist remaining items as seperate issues

Support display of higher-level ns

c761da9

gmbecker requested review from edelarua, Melkiades and ayogasekaram as code owners June 4, 2024 22:02

dependabot-preview bot and others added 3 commits June 4, 2024 22:04

[skip roxygen] [skip vbump] Roxygen Man Pages Auto Update

7b738d1

[skip style] [skip vbump] Restyle files

4f9a3f8

[skip roxygen] [skip vbump] Roxygen Man Pages Auto Update

4c7d5ca

gmbecker added 2 commits June 4, 2024 15:22

add news entries

1e56f80

Merge branch '135_higher_level_ns' of github.com:insightsengineering/…

87ac9e8

…rtables into 135_higher_level_ns

gmbecker and others added 2 commits June 4, 2024 16:46

clean up unused code, a few new tests

819f249

[skip style] [skip vbump] Restyle files

e96b4f2