diff --git a/NEWS.md b/NEWS.md index bbc3bcf75..4a186ebfe 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. * Exported previously internal `RefFootnote` constructor for reference footnote objects. * 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 diff --git a/R/colby_constructors.R b/R/colby_constructors.R index 39296f206..23f77a3bd 100644 --- a/R/colby_constructors.R +++ b/R/colby_constructors.R @@ -862,10 +862,15 @@ NULL #' 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.16` this 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 -#' data splitting (via `ref_group` parameters in, e.g., [split_rows_by()]). Similarly, if no `alt_counts_df` is -#' provided to [build_table()], `.alt_df_row` and `.alt_df` will not be present. +#' data splitting (via `ref_group` parameters in, e.g., [split_rows_by()]). Arguments derived from +#' the alt_counts_df will be passed subsets of the primary data (`df`) when no `alt_counts_df` is provided +#' to [build_table()]. #' #' @name additional_fun_params NULL diff --git a/R/tt_dotabulation.R b/R/tt_dotabulation.R index 30d6e833a..0abf96338 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) @@ -1477,14 +1484,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..3cba16e3a 100644 --- a/man/additional_fun_params.Rd +++ b/man/additional_fun_params.Rd @@ -39,10 +39,15 @@ where the variables required for row splitting are not present in \code{alt_coun \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.16} this 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, \code{.ref_group} will be missing if no baseline is previously defined during -data splitting (via \code{ref_group} parameters in, e.g., \code{\link[=split_rows_by]{split_rows_by()}}). Similarly, if no \code{alt_counts_df} is -provided to \code{\link[=build_table]{build_table()}}, \code{.alt_df_row} and \code{.alt_df} will not be present. +data splitting (via \code{ref_group} parameters in, e.g., \code{\link[=split_rows_by]{split_rows_by()}}). Arguments derived from +the alt_counts_df will be passed subsets of the primary data (\code{df}) when no \code{alt_counts_df} is provided +to \code{\link[=build_table]{build_table()}}. } diff --git a/tests/testthat/test-tab_afun_cfun.R b/tests/testthat/test-tab_afun_cfun.R index 747a17b03..07b471ec0 100644 --- a/tests/testthat/test-tab_afun_cfun.R +++ b/tests/testthat/test-tab_afun_cfun.R @@ -164,12 +164,6 @@ test_that(".spl_context and afun extra parameters contain information about comb add_overall_col("All Patients 2") |> analyze(vars = "BMRKR1", afun = n_wrapper_alt_df(ex_adsl)) - # 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) expect_silent(cbind_rtables(tbl, tbl)) @@ -352,10 +346,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 +374,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_identical(.alt_df_full, ex_adsl) ## NB true full dataset hardcoded here + 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)) +})