1095 intermediate nesting - #1111
Conversation
…1095_intermediate_nesting
…1109) - Redocument using the up to date roxygen 8.0.0 (reverting most of the changes) - Added tests for `at_sibling` - Fixed multivariable label validation and made cut-split labels visible only for `at_sibling` branches. --------- Signed-off-by: David Muñoz Tord <david.munoztord@mailbox.org> Co-authored-by: Joe Zhu <joe.zhu@roche.com> Co-authored-by: shajoezhu <3692541+shajoezhu@users.noreply.github.com>
…1095_intermediate_nesting
|
@munoztd0 I've moved your tests to test-nesting.R along with the ones I had locally, can you please convert the ones you have to use the |
|
Code Coverage SummaryDiff against mainResults for commit: 7814d28 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
…verse/rtables into 1095_intermediate_nesting
Unit Tests Summary 1 files 32 suites 2m 3s ⏱️ Results for commit 7814d28. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit d7f2423 ♻️ This comment has been updated with latest results. |
Co-authored-by: shajoezhu <3692541+shajoezhu@users.noreply.github.com>
Co-authored-by: shajoezhu <3692541+shajoezhu@users.noreply.github.com>
munoztd0
left a comment
munoztd0
left a comment
There was a problem hiding this comment.
finished last tweaks and adress comments
Of course just launched the PR -> insightsengineering/scda.test#245 |
Melkiades
left a comment
Melkiades
left a comment
There was a problem hiding this comment.
Nice work, the at_sibling feature does what #1095 asked and the happy path is solid. Verified it builds correctly and the moved column-nesting test still checks out. A couple of real bugs on the error paths though, plus some cleanup. Comments inline.
| sib_matches <- is.null(at_sibling) || at_sibling == first_spl_name(lastel) | ||
| if (endontree && sib_matches) { | ||
| splvec[[branch_pos]] <- SplitVectorTree(lst = c(lastel, list(SplitVector(newspl)))) | ||
| } else if (has_force_pag(lastel)) { |
There was a problem hiding this comment.
This crashes on the exact case the docs say is unsupported. If at_sibling targets something that was itself placed with at_sibling, lastel is a SplitVectorTree and has_force_pag has no method for that class, so you get an internal dispatch error instead of the informative message.
basic_table() |>
split_rows_by("STRATA1") |>
split_rows_by("SEX") |> analyze("AGE") |>
split_rows_by("RACE", at_sibling = "SEX") |> analyze("AGE") |>
split_rows_by("BMRKR2", at_sibling = "RACE") |> analyze("AGE") |>
build_table(ex_adsl)
#> unable to find an inherited method for 'has_force_pag' for signature 'SplitVectorTree'Needs an up-front guard with the documented error, or a has_force_pag method for SplitVectorTree. Worth a test too.
| splvec[[branch_pos]] <- SplitVectorTree(lst = c(lastel, list(SplitVector(newspl)))) | ||
| } else if (has_force_pag(lastel)) { | ||
| stop( | ||
| "at_sibling pointed to a split with forced pagination (page_by = TRUE).", |
There was a problem hiding this comment.
This guard isn't actually reached when the target uses page_by = TRUE. Instead of this message you get 'Got a page title prefix for an Elementary Table':
basic_table() |> split_rows_by("STRATA1", page_by = TRUE) |>
split_rows_by("RACE", at_sibling = "STRATA1") |> analyze("AGE") |>
build_table(ex_adsl)Would be good to make the intended error fire, with a test.
| alt_df = alt_df, | ||
| alt_df_full = alt_df_full, | ||
| splvec = splvecii, | ||
| name = obj_name(unlist(splvecii, recursive = TRUE)[[1]]), ## XXX I think this is wrong |
There was a problem hiding this comment.
These XXX markers are in the tabulation core (name= here, no_outer_tbl = TRUE just below and at 1245). Since this drives the actual table build, can we resolve them or pin the assumption with a test rather than shipping the uncertainty?
| #' to the *split* or *group of sibling analyses*, for `split_rows_by*` and | ||
| #' `analyze*` when analyzing more than one variable, respectively. Ignored when | ||
| #' analyzing a single variable. | ||
| #' @param at_sibling (`charactere(1)` or `NULL`)\cr If non-null, a preceding |
There was a problem hiding this comment.
typo: charactere(1) -> character(1). This leaks into the generated Rd files.
| setMethod( | ||
| "split_rows", "ANY", | ||
| function(lyt, spl, pos, at_sibling = NULL) { | ||
| stop("nope. can't add a row split to that (", class(lyt), "). contact the maintaner.") |
There was a problem hiding this comment.
typo in the user-facing message: maintaner -> maintainer (also lines 326, 479, 549, and AnalzyeMultiVars in the comment at 453). Spell-check CI may catch these.
|
|
||
| ## i <- 1 | ||
| ## found <- numeric() | ||
| ## while (i <= length(nmlst) && !found) { |
There was a problem hiding this comment.
Leftover commented-out while loop from the old implementation. Can drop it now that the vapply version above replaces it.
Intermediate nesting behavior, testing and documentation