Skip to content

1089 always avail alt df #1102

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.

Sign up for GitHub

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

Open
gmbecker wants to merge 4 commits into
base: main
Choose a base branch
from
Open

1089 always avail alt df #1102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
096f46b
first pass no tests yet
gmbecker Jul 24, 2026
102b0d9
remove superseded error, add docs and tests
gmbecker Jul 31, 2026
52e164d
Merge branch 'main' into 1089_always_avail_alt_df
gmbecker Jul 31, 2026
406bcf9
[skip style] [skip vbump] Restyle files
github-actions[bot] Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion R/colby_constructors.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. }

@Melkiades Melkiades Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@gmbecker small thing on the version here,I checked the tags and the stop() was actually still in v0.6.16, it is only being removed in this PR. So <= 0.6.13 is not right, it should point to the last release before this fix (0.6.16) or just say "in previous versions" to be safe. Also there is a double space and the closing } got glued onto the sentence (error. }), and this paragraph is sitting inside the \describe{} block, so it will render as a weird list item. I would move it out just after the closing brace so it reads as body text.

#'
#' @note If any of these formals is specified incorrectly or not present in the tabulation machinery, it will be

@Melkiades Melkiades Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Related to the note just below (line 1056): it still says .alt_df_row and .alt_df "will not be present" when no alt_counts_df is given, but that is exactly what we are changing here, so it now contradicts the paragraph right above. Can we update/drop it? Otherwise it is a bit confusing for the user ;)

#' treated as if missing. For example, `.ref_group` will be missing if no baseline is previously defined during
Expand Down
41 changes: 20 additions & 21 deletions R/tt_dotabulation.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand All @@ -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))
Expand Down Expand Up @@ -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
Expand All @@ -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)

Expand Down Expand Up @@ -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,
Expand Down
6 changes: 5 additions & 1 deletion man/additional_fun_params.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 28 additions & 7 deletions tests/testthat/test-tab_afun_cfun.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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*"
)

@Melkiades Melkiades Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor leftover: removing the old expect_error left a dangling blank line here, the keep_levels comment now floats away from the build_table call it refers to. Just yo tidy up.

tbl <- lyt |> build_table(DM, alt_counts_df = ex_adsl)

Expand Down Expand Up @@ -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*"
Expand Down Expand Up @@ -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))

@Melkiades Melkiades Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice test. One thing, here we only check .alt_df_full is not NULL, but the whole point is that it is the full df (not the row-group subset). I would add an expect_identical(.alt_df_full, ex_adsl) like the existing "full alt_counts_df is accessible" test does, otherwise a regression where it collapses to the subset would still pass 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))
})
Loading