diff --git a/NEWS.md b/NEWS.md index 3a0653646..891dfe9c1 100644 --- a/NEWS.md +++ b/NEWS.md @@ -5,6 +5,7 @@ * Exported previously internal `make_subset_expr` for use when constructing custom splitting behavior * Exported previously internal `splv_extra` and `splv_extra<-` accessors for getting and setting child-specific extra arguments on `SplitValue` objects. * Exported previously internal `value_expr` accessor for retrieving the subsetting expression from a `SplitValue` or `ValueWrapper` object. + * The `.alt_df*` family of afun arguments now receive subsets of `df` when `alt_counts_df` is not specified in the `build_table` call; previously resulted in an error. * Added accessor methods for `RowsVerticalSection objects`: `row_cells`, `obj_format`, `obj_format<-`, `obj_na_str`, `obj_na_str<-`, `cell_values` * Added `c` method for directly combining `RowsVerticalSection` objects * Added vignette: Guided Tour (Advanced) @gmbecker diff --git a/R/colby_constructors.R b/R/colby_constructors.R index d54c927bf..efe056a27 100644 --- a/R/colby_constructors.R +++ b/R/colby_constructors.R @@ -1044,7 +1044,11 @@ NULL #' \item{.all_col_exprs}{List of expressions. Each of them represents a different column splitting.} #' \item{.all_col_counts}{Vector of integers. Each of them represents the global count for each column. It differs #' if `alt_counts_df` is used (see [build_table()]).} -#' } +#' +#' For the `.alt_df*` family of parameters, these will be passed data +#' subsets based on `df` if no `alt_counts_df` is specified in the +#' `build_table` call. In `rtables` versions `<= 0.6.13`, this instead +#' resulted in an error. } #' #' @note If any of these formals is specified incorrectly or not present in the tabulation machinery, it will be #' treated as if missing. For example, `.ref_group` will be missing if no baseline is previously defined during diff --git a/R/tt_dotabulation.R b/R/tt_dotabulation.R index 36d8eb7b1..be8cdcf27 100644 --- a/R/tt_dotabulation.R +++ b/R/tt_dotabulation.R @@ -22,7 +22,13 @@ match_extra_args <- function(f, .N_row = .N_row, .df_row = .df_row, .all_col_exprs = .all_col_exprs, - .all_col_counts = .all_col_counts + .all_col_counts = .all_col_counts, + ## always available as of fix for https://github.com/insightsengineering/rtables/issues/1089 + ## still will only be passed to afun if it's asked for by the formals, same as + ## .N_col, etc + .alt_df = .alt_df, + .alt_df_row = .alt_df_row, + .alt_df_full = .alt_df_full ), extras ) @@ -35,16 +41,6 @@ match_extra_args <- function(f, if (!is.null(.ref_group)) { possargs <- c(possargs, list(.ref_group = .ref_group)) } - if (!is.null(.alt_df_row)) { - possargs <- c(possargs, list(.alt_df_row = .alt_df_row)) - } - if (!is.null(.alt_df)) { - possargs <- c(possargs, list(.alt_df = .alt_df)) - } - - if (!is.null(.alt_df_full)) { - possargs <- c(possargs, list(.alt_df_full = .alt_df_full)) - } if (!is.null(.ref_full)) { possargs <- c(possargs, list(.ref_full = .ref_full)) @@ -119,6 +115,9 @@ gen_onerv <- function(csub, col, count, cextr, cpath, dat <- dat[!is.na(dat[[col]]), , drop = FALSE] } + ## firstarg will be df or x (col vec), dat will always be the df + firstarg <- dat + fullrefcoldat <- cextr$.ref_full if (!is.null(fullrefcoldat)) { cextr$.ref_full <- NULL @@ -133,11 +132,19 @@ gen_onerv <- function(csub, col, count, cextr, cpath, ## behavior for x/df and ref-data (full and group) ## match if (!is.null(col) && !takesdf) { - dat <- dat[[col]] + firstarg <- firstarg[[col]] fullrefcoldat <- fullrefcoldat[[col]] baselinedf <- baselinedf[[col]] } - args <- list(dat) + args <- list(firstarg) + + ## replace alt_df (potential) args with their df versions if alt_counts_df not set + ## in build_table call + if (is.null(alt_df_full)) { + alt_df_full <- if (NROW(spl_context) > 0) spl_context$full_parent_df[[1]] else dfpart + alt_dfpart <- dfpart + alt_dfpart_fil <- dat + } names(all_col_counts) <- names(all_col_exprs) @@ -1405,14 +1412,6 @@ build_table <- function(lyt, df, lyt <- set_def_child_ord(lyt, df) lyt <- fix_analyze_vis(lyt) df <- fix_split_vars(lyt, df, char_ok = is.null(col_counts)) - alt_params <- check_afun_cfun_params(lyt, c(".alt_df", ".alt_df_row")) - if (any(alt_params) && is.null(alt_counts_df)) { - stop( - "Layout contains afun/cfun functions that have optional parameters ", - ".alt_df and/or .alt_df_row, but no alt_counts_df was provided in ", - "build_table()." - ) - } rtpos <- TreePos() cinfo <- create_colinfo(lyt, df, rtpos, diff --git a/man/additional_fun_params.Rd b/man/additional_fun_params.Rd index 062520343..7785e5db0 100644 --- a/man/additional_fun_params.Rd +++ b/man/additional_fun_params.Rd @@ -38,7 +38,11 @@ where the variables required for row splitting are not present in \code{alt_coun \item{.all_col_exprs}{List of expressions. Each of them represents a different column splitting.} \item{.all_col_counts}{Vector of integers. Each of them represents the global count for each column. It differs if \code{alt_counts_df} is used (see \code{\link[=build_table]{build_table()}}).} -} + +For the \verb{.alt_df*} family of parameters, these will be passed data +subsets based on \code{df} if no \code{alt_counts_df} is specified in the +\code{build_table} call. In \code{rtables} versions \verb{<= 0.6.13}, this instead +resulted in an error. } } \note{ If any of these formals is specified incorrectly or not present in the tabulation machinery, it will be diff --git a/tests/testthat/test-tab_afun_cfun.R b/tests/testthat/test-tab_afun_cfun.R index 747a17b03..55d40dc38 100644 --- a/tests/testthat/test-tab_afun_cfun.R +++ b/tests/testthat/test-tab_afun_cfun.R @@ -166,9 +166,6 @@ test_that(".spl_context and afun extra parameters contain information about comb # NB: If you add keep_levels = c("all_X") to add_combo_levels the other # column expressions are missing -> Expected! - expect_error(lyt |> build_table(DM), - regexp = "Layout contains afun\\/cfun functions that have optional*" - ) tbl <- lyt |> build_table(DM, alt_counts_df = ex_adsl) @@ -352,10 +349,6 @@ test_that(".alt_df_row appears in cfun but not in afun.", { split_rows_by("ARMCD") |> analyze("BMRKR1", afun = afun_tmp) - expect_error( - lyt |> build_table(ex_adsl), - "Layout contains afun/cfun functions that have optional*" - ) expect_error( lyt |> build_table(ex_adsl, alt_counts_df = DM), "alt_counts_df appears incompatible with column-split*" @@ -384,3 +377,31 @@ test_that("full alt_counts_df is accessible from afun/cfun via .alt_df_full", { cvals <- unlist(cell_values(tbl)) expect_true(all(cvals == "ok")) }) + +test_that(".alt_df* argument behavior is correct when alt_counts_df is not set", { + check_alt_dfs <- function(df, .df_row, .alt_df_row, .alt_df, .alt_df_full) { + expect_identical(df, .alt_df) + expect_identical(.df_row, .alt_df_row) + expect_false(is.null(.alt_df_full)) + TRUE + } + + afun <- function(df, .df_row, .alt_df_row, .alt_df, .alt_df_full) { + res <- check_alt_dfs(df, .df_row, .alt_df_row, .alt_df, .alt_df_full) + in_rows("afun result" = "OK") + } + cfun <- function(df, labelstr, .df_row, .alt_df_row, .alt_df, .alt_df_full) { + res <- check_alt_dfs(df, .df_row, .alt_df_row, .alt_df, .alt_df_full) + in_rows("cfun result" = "OK", .formats = list("cfun result" = "xx")) + } + + first_2_levs <- function(vec) levels(vec)[1:2] + + lyt <- basic_table() |> + split_cols_by("ARM", split_fun = keep_split_levels(first_2_levs(ex_adsl$ARM))) |> + split_rows_by("STRATA1", split_fun = keep_split_levels(first_2_levs(ex_adsl$STRATA1))) |> + summarize_row_groups("STRATA1", cfun = cfun) |> + analyze("AGE", afun = afun) + + expect_no_error(build_table(lyt, ex_adsl)) +})