Skip to content

use dplyr::bind_rows in ard_stack instead of bind_ard and remove ard_stack class - #592

Open
alanahjonas95 wants to merge 9 commits into
mainfrom
586_bind_rows
Open

use dplyr::bind_rows in ard_stack instead of bind_ard and remove ard_stack class#592
alanahjonas95 wants to merge 9 commits into
mainfrom
586_bind_rows

Conversation

@alanahjonas95

@alanahjonas95 alanahjonas95 commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

What changes are proposed in this pull request?

  • Style this entry in a way that can be copied directly into NEWS.md. (#, @)

Update ard_stack to use dplyr::bind_rows instead of bind_ard, and remove ard_stack class

Provide more detail here as needed.

Reference GitHub issue associated with pull request. e.g., 'closes #'

closes #586

Pre-review Checklist (if item does not apply, mark is as complete)

  • All GitHub Action workflows pass with a ✅
  • PR branch has pulled the most recent updates from master branch: usethis::pr_merge_main()
  • If a bug was fixed, a unit test was added.
  • Code coverage is suitable for any new functions/features (generally, 100% coverage for new code): devtools::test_coverage()
  • Request a reviewer

Reviewer Checklist (if item does not apply, mark is as complete)

  • If a bug was fixed, a unit test was added.
  • Run pkgdown::build_site(). Check the R console for errors, and review the rendered website.
  • Code coverage is suitable for any new functions/features: devtools::test_coverage()

When the branch is ready to be merged:

  • Update NEWS.md with the changes from this pull request under the heading "# cards (development version)". If there is an issue associated with the pull request, reference it in parentheses at the end update (see NEWS.md for examples).
  • All GitHub Action workflows pass with a ✅
  • Approve Pull Request
  • Merge the PR. Please use "Squash and merge" or "Rebase and merge".

Optional Reverse Dependency Checks:

Install checked with pak::pak("Genentech/checked") or pak::pak("checked")

# Check dev versions of `cardx`, `gtsummary`, and `tfrmt` which are in the `ddsjoberg` R Universe
Rscript -e "options(checked.check_envvars = c(NOT_CRAN = TRUE)); checked::check_rev_deps(path = '.', n = parallel::detectCores() - 2L, repos = c('https://ddsjoberg.r-universe.dev', 'https://cloud.r-project.org'))"

# Check CRAN reverse dependencies but run tests skipped on CRAN
Rscript -e "options(checked.check_envvars = c(NOT_CRAN = TRUE)); checked::check_rev_deps(path = '.', n = parallel::detectCores() - 2, repos = 'https://cloud.r-project.org')"

# Check CRAN reverse dependencies in a CRAN-like environment
Rscript -e "options(checked.check_envvars = c(NOT_CRAN = FALSE), checked.check_build_args = '--as-cran'); checked::check_rev_deps(path = '.', n = parallel::detectCores() - 2, repos = 'https://cloud.r-project.org')"

@alanahjonas95
alanahjonas95 requested a review from bzkrouse August 6, 2026 15:02
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Unit Tests Summary

  1 files  277 suites   25s ⏱️
277 tests 181 ✅  96 💤 0 ❌
625 runs  496 ✅ 129 💤 0 ❌

Results for commit 2b40553.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

badge

Code Coverage Summary

Filename                       Stmts    Miss  Cover    Missing
---------------------------  -------  ------  -------  ---------------------------------------------------------------------------------------------------------------------------------------
R/add_calculated_row.R            53       6  88.68%   51-56
R/apply_fmt_fun.R                116       0  100.00%
R/ard_attributes.R                46       1  97.83%   57
R/ard_formals.R                   13       0  100.00%
R/ard_hierarchical.R             104      12  88.46%   88-93, 187-192
R/ard_identity.R                  13       0  100.00%
R/ard_missing.R                   70       7  90.00%   45-50, 61
R/ard_mvsummary.R                 54       7  87.04%   76-81, 100
R/ard_pairwise.R                  50       0  100.00%
R/ard_stack_hierarchical.R       272       0  100.00%
R/ard_stack.R                     99       0  100.00%
R/ard_strata.R                    38       0  100.00%
R/ard_summary.R                  200       8  96.00%   87-92, 226-227
R/ard_tabulate_rows.R             12       0  100.00%
R/ard_tabulate_value.R            85       8  90.59%   51-56, 73, 97
R/ard_tabulate.R                 629     103  83.62%   108-113, 194, 271, 310, 400, 412-415, 547, 607-612, 643, 819-823, 829-842, 873, 880-885, 889-908, 944-954, 958-977, 981-985, 1015, 1048
R/ard_total_n.R                   19       0  100.00%
R/as_card_fn.R                     8       0  100.00%
R/as_card.R                       14       0  100.00%
R/as_nested_list.R                41       0  100.00%
R/bind_ard.R                      49      11  77.55%   74-85
R/cards-package.R                  1       1  0.00%    14
R/check_ard_structure.R           55       5  90.91%   35-39
R/compare_ard_helpers.R          127      32  74.80%   44-50, 66-69, 113, 148, 153, 158-184, 227, 230
R/compare_ard.R                   26       0  100.00%
R/default_stat_labels.R           20       0  100.00%
R/deprecated.R                    34      30  11.76%   40-41, 56-73, 88-134
R/eval_capture_conditions.R       30       0  100.00%
R/filter_ard_hierarchical.R      209       0  100.00%
R/get_ard_statistics.R            20       0  100.00%
R/maximum_variable_value.R        13       0  100.00%
R/mock.R                         137       2  98.54%   116, 245
R/nest_for_ard.R                  80       1  98.75%   64
R/print_ard_conditions.R          83       0  100.00%
R/print.R                         77       2  97.40%   190-191
R/process_selectors.R            116       1  99.14%   326
R/rename_ard_columns.R            84       0  100.00%
R/rename_ard_groups.R             60       0  100.00%
R/replace_null_statistic.R        11       0  100.00%
R/round5.R                         1       0  100.00%
R/selectors.R                     23       0  100.00%
R/shuffle_ard.R                  167       0  100.00%
R/sort_ard_hierarchical.R        195       0  100.00%
R/summary_functions.R             25       1  96.00%   59
R/tidy_ard_order.R                42       0  100.00%
R/tidy_as_ard.R                   40       0  100.00%
R/unlist_ard_columns.R            28       0  100.00%
R/update_ard.R                    60       6  90.00%   54-59
R/utils.R                         32       1  96.88%   37
TOTAL                           3781     245  93.52%

Diff against main

Filename         Stmts    Miss  Cover
-------------  -------  ------  --------
R/ard_stack.R       +1       0  +100.00%
TOTAL               +1       0  +0.00%

Results for commit: fcf9bf7

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
ard_summary 💔 $2.31$ $+1.08$ ard_summary_works
ard_tabulate 💚 $1.75$ $-1.23$ ard_tabulate_univariate

Results for commit f40c2f9

♻️ This comment has been updated with latest results.

Comment thread R/ard_stack.R
Comment on lines -13 to -15
# Verify ard_stack class is assigned and ordered correctly
expect_s3_class(ard1, "ard_stack")
expect_identical(class(ard1)[1], "ard_stack")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

let's add this back as well, thanks

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

added these tests back in

ddsjoberg and others added 6 commits August 6, 2026 13:24
Signed-off-by: Alanah Jonas <156101954+alanahjonas95@users.noreply.github.com>
Signed-off-by: Alanah Jonas <156101954+alanahjonas95@users.noreply.github.com>
Signed-off-by: Alanah Jonas <156101954+alanahjonas95@users.noreply.github.com>
Signed-off-by: Alanah Jonas <156101954+alanahjonas95@users.noreply.github.com>
Signed-off-by: Alanah Jonas <156101954+alanahjonas95@users.noreply.github.com>
@alanahjonas95

Copy link
Copy Markdown
Collaborator Author

@ddsjoberg i do have one question/issue about this change to use bind_rows though, currently bind_ard removes duplicate rows, but we lose that funcionality if we use bind_rows instead,

i know this is a bad example / probably not something someone would do:

current messaging:
image

but after this update all the rows are kept :
image

@alanahjonas95

Copy link
Copy Markdown
Collaborator Author

@ddsjoberg, @bzkrouse and i think we should close this pr as there isnt a benefit to converting to bind_rows for tfrmt if we are going to keep the class,
so are you ok if we close this?

@ddsjoberg

Copy link
Copy Markdown
Collaborator

@ddsjoberg, @bzkrouse and i think we should close this pr as there isnt a benefit to converting to bind_rows for tfrmt if we are going to keep the class, so are you ok if we close this?

Up to you and @bzkrouse !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

consider using bind_rows under the hood of ard_stack in lieu of bind_ard

2 participants