Skip to content

feat: accept svyrep.design in the survey test and CI functions - #357

Open
szimmer wants to merge 1 commit into
pharmaverse:mainfrom
szimmer:355_svyrep_class_guards
Open

feat: accept svyrep.design in the survey test and CI functions#357
szimmer wants to merge 1 commit into
pharmaverse:mainfrom
szimmer:355_svyrep_class_guards

Conversation

@szimmer

@szimmer szimmer commented Aug 30, 2026

Copy link
Copy Markdown

What changes are proposed in this pull request?

  • ard_survey_svychisq(), ard_survey_svyttest(), ard_survey_svyranktest(), ard_categorical_ci() and ard_continuous_ci() now accept replicate-weight survey designs (svyrep.design). These functions previously rejected such designs with a class check, although the survey functions they wrap already support them. This is what gtsummary::add_p() and gtsummary::add_ci() require: because add_p() traps errors per variable, the rejection surfaced to users as a silently empty p-value column rather than an error. (Feature Request: Support svyrep.design (replicate-weight) survey objects #355, @szimmer)

Provide more detail here as needed.

This is the second of the two blockers described in #355, and follows #356, which added the missing svyrep.design methods for the six summary ard_* generics.

The guards were stricter than the code they wrap. Each of the five functions carried check_class(data, "survey.design"), but the underlying survey functions handle replicate designs already — svychisq(), svyranktest() and svyquantile() have dedicated svyrep.design methods, and svyttest()/svyciprop() are not generic and are design-agnostic:

svychisq(~stype + both, des)  #> p = 0.06135439
svychisq(~stype + both, rep)  #> p = 0.07992227

ard_categorical_ci() and ard_continuous_ci() also needed svyrep.design methods registered, since they are S3 methods rather than plain functions — relaxing the guard alone would not route a replicate design to them. I placed those two methods alongside their survey.design counterparts rather than in R/svyrep.design.R, so this branch does not conflict with #356. Happy to consolidate them into that file once both are merged, if you'd prefer them together.

The user-visible effect. gtsummary::add_p() traps errors per variable, so the class guard did not reach users as a message — it produced a table with an empty p-value column. With both PRs in place:

before after
add_p(), replicate design NA 0.07992227
svychisq() called directly 0.07992227 matches
add_ci(), replicate design error 588, 701

Tests (tests/testthat/test-svyrep.design_tests_ci.R) assert for each function that the result matches survey computed directly on the replicate design, and that it differs from the same computation on the linearized design. A silent fallback to linearization would therefore fail rather than pass. The class guards are also tested to still reject non-survey input.

Not included. ard_emmeans_contrast() and ard_emmeans_emmeans() carry the same guard and are left for a follow-up — I have not yet tested emmeans against replicate designs, and did not want that holding up the five functions I can evidence. gtsummary::add_difference() will therefore still fail on replicate designs after this merges.

Reference GitHub issue associated with pull request.

Addresses #355. Together with #356 this covers both blockers described there, apart from the ard_emmeans_* pair noted above.


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.
  • If a new ard_*() function was added, it passes the ARD structural checks from cards::check_ard_structure().
  • If a new ard_*() function was added, set_cli_abort_call() has been set.
  • If a new ard_*() function was added and it depends on another package (such as, broom), is_pkg_installed("broom") has been set in the function call and the following added to the roxygen comments: @examplesIf do.call(asNamespace("cardx")$is_pkg_installed, list(pkg = "broom""))
  • Code coverage is suitable for any new functions/features (generally, 100% coverage for new code): devtools::test_coverage()

Notes on the checklist:

  • No new exported functions are added; the two new S3 methods delegate to their survey.design counterparts, so set_cli_abort_call() and the is_pkg_installed() checks are inherited.
  • covr hits every line this PR changes (7/7). Per file: ard_continuous_ci.survey.design.R 100%, ard_categorical_ci.survey.design.R 96.67%, ard_survey_svyttest.R 93.75%, ard_survey_svyranktest.R 93.33%, ard_survey_svychisq.R 91.67% — the shortfalls are pre-existing uncovered lines this PR does not touch. Package coverage is 96.73%, unchanged from main.
  • lintr reports no findings on any changed line.

`ard_survey_svychisq()`, `ard_survey_svyttest()`, `ard_survey_svyranktest()`,
`ard_categorical_ci()` and `ard_continuous_ci()` rejected replicate-weight
designs with a `check_class(data, "survey.design")` guard, even though the
`survey` functions they wrap already support them:

    svychisq(~stype + both, des)  #> p = 0.06135439
    svychisq(~stype + both, rep)  #> p = 0.07992227

Relax those five guards to accept `svyrep.design`, and register
`svyrep.design` methods for `ard_categorical_ci()` and `ard_continuous_ci()`,
which are S3 methods rather than plain functions -- relaxing the guard alone
would not route a replicate design to them.

The new methods are placed alongside their `survey.design` counterparts rather
than in R/svyrep.design.R, to avoid a merge conflict with the PR that adds
that file.

This is what gtsummary's add_p() and add_ci() need. Because add_p() traps
errors per variable, the guard previously surfaced to users not as an error
but as a silently empty p-value column. With both halves in place add_p()
returns 0.07992227 for the replicate design above, matching svychisq()
directly, where it previously returned NA.

Tests assert that each result matches `survey` computed directly on the
replicate design, and differs from the linearized design, so a silent fallback
to linearization would fail rather than pass.

Refs pharmaverse#355

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant