feat: accept svyrep.design in the survey test and CI functions - #357
Open
szimmer wants to merge 1 commit into
Open
feat: accept svyrep.design in the survey test and CI functions#357szimmer wants to merge 1 commit into
szimmer wants to merge 1 commit into
Conversation
`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>
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
ard_survey_svychisq(),ard_survey_svyttest(),ard_survey_svyranktest(),ard_categorical_ci()andard_continuous_ci()now accept replicate-weight survey designs (svyrep.design). These functions previously rejected such designs with a class check, although thesurveyfunctions they wrap already support them. This is whatgtsummary::add_p()andgtsummary::add_ci()require: becauseadd_p()traps errors per variable, the rejection surfaced to users as a silently empty p-value column rather than an error. (Feature Request: Supportsvyrep.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.designmethods for the six summaryard_*generics.The guards were stricter than the code they wrap. Each of the five functions carried
check_class(data, "survey.design"), but the underlyingsurveyfunctions handle replicate designs already —svychisq(),svyranktest()andsvyquantile()have dedicatedsvyrep.designmethods, andsvyttest()/svyciprop()are not generic and are design-agnostic:ard_categorical_ci()andard_continuous_ci()also neededsvyrep.designmethods 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 theirsurvey.designcounterparts rather than inR/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:add_p(), replicate designNAsvychisq()called directlyadd_ci(), replicate designTests (
tests/testthat/test-svyrep.design_tests_ci.R) assert for each function that the result matchessurveycomputed 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()andard_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)
usethis::pr_merge_main()ard_*()function was added, it passes the ARD structural checks fromcards::check_ard_structure().ard_*()function was added,set_cli_abort_call()has been set.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""))devtools::test_coverage()Notes on the checklist:
survey.designcounterparts, soset_cli_abort_call()and theis_pkg_installed()checks are inherited.covrhits every line this PR changes (7/7). Per file:ard_continuous_ci.survey.design.R100%,ard_categorical_ci.survey.design.R96.67%,ard_survey_svyttest.R93.75%,ard_survey_svyranktest.R93.33%,ard_survey_svychisq.R91.67% — the shortfalls are pre-existing uncovered lines this PR does not touch. Package coverage is 96.73%, unchanged frommain.lintrreports no findings on any changed line.