Skip to content
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

Fix potential tt at path issue #794

Merged
merged 6 commits into from
Nov 29, 2023
Merged

Fix potential tt at path issue #794

merged 6 commits into from
Nov 29, 2023

Conversation

clarkliming
Copy link
Contributor

there is a fix for path names

path <- unname(rdf$path[[i]])

however tt_at_path is still not updated. It is not robust enough.

library(tern)
library(dplyr)
adtte_f <- tern_ex_adtte %>%
  filter(PARAMCD == "OS") %>%
  mutate(
    AVAL = day2month(AVAL),
    is_event = CNSR == 0
  )

adtte_f$is_event <- FALSE

tbl <- basic_table() %>%
  split_cols_by(var = "ARMCD", ref_group = "ARM A") %>%
  add_colcounts() %>%
  surv_time(
    vars = "AVAL",
    var_labels = "Survival Time (Months)",
    is_event = "is_event",
    control = control_surv_time()
  ) %>%
  build_table(df = adtte_f)

tt_at_path(tbl, row_paths(tbl)[[5]])

it will give you an error that the path is not valid.

Let's fix this issue from the root. Let's not use "identical" for comparison (to avoid potential issue of character names), or let's just use unname() for all occurences of row_paths, tt_at_path, etc. ideally the row_paths should not have any name.

Copy link
Contributor

github-actions bot commented Nov 29, 2023

Unit Tests Summary

       1 files       24 suites   1m 29s ⏱️
   197 tests    197 ✔️ 0 💤 0
1 505 runs  1 505 ✔️ 0 💤 0

Results for commit 8b468b3.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Nov 29, 2023

badge

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/00tabletrees.R               744      62  91.67%   21, 102, 105, 412, 496-497, 500, 656, 757, 849-850, 951, 953-954, 977-980, 1002, 1114-1117, 1212-1217, 1365, 1462-1465, 1529-1532, 1568-1571, 1577-1582, 1632, 1639, 1739, 1852, 1866, 1869-1872, 1875-1878, 1906, 1937-1938
R/as_html.R                    161      25  84.47%   5-10, 74, 131-136, 141-146, 161-165, 253
R/colby_constructors.R         558      20  96.42%   71, 123, 181-184, 244-247, 387, 404, 1213, 1306, 1468, 1506, 1528, 1552, 1573, 1729
R/compare_rtables.R             83      17  79.52%   93-96, 99-102, 115-118, 137, 157-158, 189, 194
R/format_rcell.R                12       0  100.00%
R/indent.R                      13       2  84.62%   39-40
R/index_footnotes.R             66       0  100.00%
R/make_split_fun.R             119      23  80.67%   22-25, 51-54, 57-60, 115, 119, 280, 283-286, 291-294, 313, 412
R/make_subset_expr.R           136      14  89.71%   34-48, 126-133, 168, 250, 266, 274
R/simple_analysis.R              5       1  80.00%   55
R/split_funs.R                 505      66  86.93%   143, 148, 154-159, 164, 181-185, 366-371, 388-393, 474, 526, 544-547, 564, 631, 641-642, 644, 658, 702, 727, 903, 910, 936-939, 950-951, 953, 955, 1126-1128, 1142-1146, 1210-1213, 1276-1279
R/summary.R                    215      24  88.84%   38, 85, 192, 200, 271-276, 287-288, 307-308, 418, 465-481, 516, 549
R/tree_accessors.R             946     102  89.22%   109, 251, 269, 292, 330, 344, 360, 465, 492-493, 774-779, 907, 925, 949, 999, 1054-1055, 1094, 1127, 1163-1167, 1220, 1295-1299, 1317-1327, 1396, 1501-1504, 1529, 1549-1550, 1559, 1600, 1618-1622, 1643-1647, 1726, 1768, 1872, 1976, 1989, 2002, 2016, 2024, 2033-2037, 2379, 2737, 2850, 2883-2905, 2994-3001, 3156, 3229-3234, 3443-3444, 3451, 3454-3457, 3461, 3512, 3546, 3571-3595
R/tt_afun_utils.R              411      32  92.21%   50, 164, 171, 181-194, 260, 271-272, 504, 512-515, 597-601, 622, 637-639
R/tt_compare_tables.R           70       4  94.29%   56, 178, 257, 261
R/tt_compatibility.R           510      56  89.02%   19, 142-143, 192, 197, 332-333, 337-340, 346, 350, 398, 520, 568, 601, 621, 654-657, 701, 718-722, 809, 837-840, 849, 912, 920, 931-934, 1044, 1051, 1080-1094, 1125-1126
R/tt_dotabulation.R           1117      96  91.41%   54, 252, 257, 259, 310, 334, 338-341, 373-376, 399, 434-437, 465-468, 565, 702-706, 756, 760, 789-792, 802, 822-826, 833-836, 1095, 1099, 1130, 1243-1246, 1442-1450, 1591, 1675-1684, 1764-1767, 1778, 1783, 1788-1789, 1791, 1802, 1807, 1830, 1925-1944
R/tt_export.R                  540      39  92.78%   44, 173-177, 216-219, 261-264, 316-320, 384, 438, 524-527, 542, 886, 895, 920-924, 1091-1094, 1097, 1128, 1134
R/tt_from_df.R                   9       0  100.00%
R/tt_paginate.R                440      37  91.59%   45, 70, 107-115, 396, 518-521, 541-545, 695-698, 748-755, 824, 827, 837, 844, 847
R/tt_pos_and_access.R          571      43  92.47%   77, 79-81, 106, 166, 212-216, 262, 515, 517, 525, 531, 545, 555-558, 740, 751-755, 760-763, 790, 843-844, 856, 1022-1023, 1081-1109, 1389, 1466
R/tt_showmethods.R             144      21  85.42%   60, 97-120, 183, 209, 218, 226, 229-233, 326-327
R/tt_sort.R                     88       5  94.32%   223-226, 234
R/tt_toString.R                395      25  93.67%   115, 306, 356, 371, 381, 388, 391, 397-407, 495, 560, 566, 801-827
R/utils.R                       29       0  100.00%
R/validate_table_struct.R       84      10  88.10%   79-83, 92-93, 140, 150-151
R/Viewer.R                      61       9  85.25%   47, 51, 61-65, 85, 119
TOTAL                         8032     733  90.87%

Diff against main

Filename                     Stmts    Miss  Cover
-------------------------  -------  ------  --------
R/00tabletrees.R              +120     +21  -1.76%
R/as_html.R                    +73     +18  -7.57%
R/colby_constructors.R         +76      +3  -0.06%
R/compare_rtables.R             +5      +6  -6.38%
R/index_footnotes.R            +16       0  +100.00%
R/make_split_fun.R            +119     +23  +80.67%
R/make_subset_expr.R           +30      +2  +1.03%
R/split_funs.R                 +84     +11  -0.01%
R/summary.R                    +32      +8  -2.42%
R/tree_accessors.R            +153     +34  -2.21%
R/tt_afun_utils.R              +66      +7  -0.54%
R/tt_compare_tables.R           +5       0  +0.44%
R/tt_compatibility.R           +97      +6  +1.13%
R/tt_dotabulation.R           +380     +52  -2.62%
R/tt_export.R                 +311     -34  +24.66%
R/tt_paginate.R                +57     +22  -4.49%
R/tt_pos_and_access.R          +51      +6  -0.42%
R/tt_showmethods.R             +23       0  +2.77%
R/tt_sort.R                     +7      -1  +1.73%
R/tt_toString.R                +90      +3  +0.88%
R/utils.R                      +18      -1  +9.09%
R/validate_table_struct.R      +84     +10  +88.10%
R/Viewer.R                      +5       0  +1.32%
TOTAL                        +1902    +196  -0.37%

Results for commit: 8b468b3

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@Melkiades Melkiades requested review from Melkiades and removed request for Melkiades, edelarua and ayogasekaram November 29, 2023 13:40
@Melkiades Melkiades self-assigned this Nov 29, 2023
Copy link
Contributor

@Melkiades Melkiades left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Liming for finding this bug! I could not reproduce it in base {rtables} but I corrected the more general row_paths() that is based on make_rows_df() so now it will not produce named paths. I checked with surv_time and cell_values which was also breaking and the latter fix works. I think it is fine not to have identical as the label is not really a needed check. I added a test but it is not generating named paths. @edelarua do you know how it is possible to correct the test to have it doing what surv_time does?

Copy link
Contributor

@edelarua edelarua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the test - this is good to go now!

@Melkiades Melkiades merged commit d861d76 into main Nov 29, 2023
17 checks passed
@Melkiades Melkiades deleted the fix_tt_at_path_issue branch November 29, 2023 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants