Repository navigation
Conversation
`svyrep.design` is a sibling class of `survey.design` rather than a subclass, so replicate-weight designs failed S3 dispatch before reaching any statistical code. Register `svyrep.design` methods for the six survey ard_* generics. The implementations are shared with the existing `survey.design` methods: statistics are computed by survey::svymean(), svytotal(), svyvar(), svyquantile(), svytable() and svyby(), all of which already handle replicate designs, so no separate calculation is needed. Tests cover dispatch, ARD structure, agreement of weighted point estimates with the linearized design, that replicate variance is genuinely used rather than silently falling back to linearization, and all replicate types (JK1, JKn, bootstrap, subbootstrap, Fay, BRR). Refs pharmaverse#355 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The methods were previously alias assignments
(`ard_tabulate.svyrep.design <- ard_tabulate.survey.design`). That form
is invisible to tooling that reasons about the source statically:
- covr reported no rows at all for R/svyrep.design.R -- the file was
absent from the coverage report rather than scoring 0% or 100%,
because top-level assignments carry no instrumentable expressions
- covr::file_coverage() errored outright with
"object 'ard_attributes.survey.design' not found", since the aliases
resolve only at load time and depend on file collation order
- static analysis could not resolve the right-hand sides
Defining each method as a thin wrapper that forwards to its
`survey.design` counterpart fixes all three: R/svyrep.design.R now
reports 100% coverage (6/6 lines hit by the test suite) and no longer
depends on files being sourced in a particular order. Behaviour is
unchanged -- the same `survey.design` implementations do the work, and
tidyselect arguments pass through `...` untouched.
Also:
- add an @examplesIf block to each of the six shared help topics
showing a replicate design built with as.svrepdesign(), and
regenerate the .Rd files
- scope the NEWS entry to the six functions actually covered, and note
that the survey test and confidence interval functions do not yet
accept replicate designs, so the release notes do not promise more
than the change delivers
- satisfy lintr in the tests: rename BRRrep to brr_rep (snake_case) and
load the survey data into an explicit environment so apiclus1 is not
flagged as an undefined global inside the helper
Refs pharmaverse#355
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`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>
`ard_emmeans_emmeans()` and `ard_emmeans_contrast()` rejected replicate-weight
designs with a class guard. Underneath that guard sat a second problem: both
selected the model data with
data_in <- if (dplyr::last(class(data)) == "survey.design") ...
which happens to identify `survey.design2` -- whose last class element is
"survey.design" -- but does not identify a `svyrep.design` at all. Relaxing
only the guard would have passed the design object where a data frame was
expected. The test is now `inherits(data, c("survey.design",
"svyrep.design"))`, which is also less brittle for `survey.design` subclasses
generally.
`construct_model()` needed a `svyrep.design` method for the same
sibling-class reason; it delegates to the `survey.design` method, and the
model is fit by `survey::svyglm()`, which already handles replicate designs.
emmeans itself required no changes -- the estimated marginal means come back
with replicate-based standard errors, differing from the linearized design
and matching `emmeans::emmeans()` applied to the replicate model directly:
linearized SEs: 28.98769 23.42657
replicate SEs: 32.48311 26.07184
Tests cover both functions, `construct_model()`, and a `data.frame` case to
confirm the changed class test did not alter the non-survey path.
Refs pharmaverse#355
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…355_svyrep_updates
ddsjoberg
left a comment
ddsjoberg
left a comment
There was a problem hiding this comment.
I took a quick look. Can you please address and I'll continue
| lme4 (>= 1.1-37), | ||
| parameters (>= 0.20.2), | ||
| smd (>= 0.6.6), | ||
| srvyr, |
There was a problem hiding this comment.
let's remove this, and drop any tests for these objects specifically. Fewer deps the better for us! (And srvry is already handling much of that testing to ensure their objects work with the survey infrastructure)
| @@ -0,0 +1,159 @@ | |||
| skip_if_pkg_not_installed("survey") | |||
There was a problem hiding this comment.
Review how the other test files are named, and follow those naming conventions/organizational structure.
| #' @rdname ard_attributes | ||
| #' @param data (`survey.design`)\cr | ||
| #' a design object often created with [`survey::svydesign()`]. | ||
| #' @param data (`survey.design`) or (`svyrep.design`)\cr |
There was a problem hiding this comment.
This is the only change for this file. Don't you also need to add an S3 method for the svrep.design objects?
| @@ -1,5 +1,7 @@ | |||
| # cardx (development version) | |||
|
|
|||
| * Added `svyrep.design` support across survey, statistical test, CI, and emmeans methods, allowing these functions to work with replicate-weight designs created by `survey::svrepdesign()` or `survey::as.svrepdesign()`. Previously, S3 dispatch and explicit class checks rejected these designs even though the underlying survey functions already supported them. Statistics continue to use the same `survey` implementations, so variance estimates correctly account for replicate weights. `construct_model()` also now supports `svyrep.design`, and emmeans methods use `inherits(data, c("survey.design", "svyrep.design"))` to correctly identify both survey design classes. Additionally, `tbl_svy` produced from the {srvyr} package are also supported because `tbl_svy` objects are also of class `survey.design` or `svyrep.design`. (#355, @szimmer) | |||
There was a problem hiding this comment.
explicit class checks rejected these designs
Update this language. We didn't explicitly reject the class.
| #' variables = c(sname, dname), | ||
| #' label = list(sname = "School Name", dname = "District Name") | ||
| #' ) | ||
| ard_attributes.svyrep.design <- function(data, ...) { |
There was a problem hiding this comment.
Ah, I see this function now. The survey methods can be documented together instead of in a separate file.
|
@szimmer Below is a Claude review of the PR. Let me know your thoughts on the raised points. Review follow-ups for #359 (
|
|
Changing this to a draft while I work on updates. Update plans include:
|
Ah, thanks for letting me know. Just updated the guide |
|
I got stuck on some tests in ard_tabulate not passing with a replicate object. No standard errors were being output. I figured out the root of the issue but trying to think of a graceful way to handle this. The svyby output isn't labeled as expected svy_titanic <- survey::svydesign(~1, data = as.data.frame(Titanic), weights = ~Freq)
rsvy_titanic <- survey::as.svrepdesign(svy_titanic)
survey::svyby(
formula = ~Class,
by = ~Survived,
design = svy_titanic,
FUN = survey::svymean,
na.rm = TRUE,
deff = TRUE
)
#> Survived Class1st Class2nd Class3rd ClassCrew se.Class1st se.Class2nd
#> No No 0.08187919 0.1120805 0.3543624 0.4516779 0.0862186 0.1113057
#> Yes Yes 0.28551336 0.1659634 0.2503516 0.2981716 0.1820787 0.1175809
#> se.Class3rd se.ClassCrew DEff.Class1st DEff.Class2nd DEff.Class3rd
#> No 0.2434649 0.2853846 0.8959744 1.127972 2.347487
#> Yes 0.1485929 0.2121769 2.1551529 1.324513 1.560158
#> DEff.ClassCrew
#> No 2.979638
#> Yes 2.852852
survey::svyby(
formula = ~Class,
by = ~Survived,
design = rsvy_titanic,
FUN = survey::svymean,
na.rm = TRUE,
deff = TRUE
)
#> Survived Class1st Class2nd Class3rd ClassCrew se1 se2
#> No No 0.08187919 0.1120805 0.3543624 0.4516779 0.1061470 0.1411537
#> Yes Yes 0.28551336 0.1659634 0.2503516 0.2981716 0.2191396 0.1332299
#> se3 se4 DEff.Class1st DEff.Class2nd DEff.Class3rd
#> No 0.3676938 0.4723809 2.272596 3.035715 8.960170
#> Yes 0.1710918 0.2746881 3.612413 1.967805 2.393461
#> DEff.ClassCrew
#> No 13.661553
#> Yes 5.532971So then this doesn't work as intended: |
|
@szimmer I recall when looking at this the first time that some of the output changed labels, and I just got overwhelmed and stopped looking at it. These survey objects are hard to work with. I think we'll need tests for every summary type with and without |
| 2 api00 survey_continuous_ci std.error std.error 23.54224 2 | ||
| 3 api00 survey_continuous_ci conf.low conf.low 593.6763 2 | ||
| 4 api00 survey_continuous_ci conf.high conf.high 694.6625 2 | ||
| 1 api00 survey_continuous_ci estimate estimate 634.96 2 |
There was a problem hiding this comment.
why are so many numbers changing in the snapshot?
There was a problem hiding this comment.
To make the tests run faster - I subset the api data to be smaller - https://github.com/pharmaverse/cardx/pull/359/changes#diff-7d19f54ffc8b540b9abe12066e988386b33c41569ff83e136a16afe412e94b2aR4
I did this across several tests. We can roll it back as it's in an isolated commit.
There was a problem hiding this comment.
We can't change the existing tests so we can assess whether the current changes break existing code.
There was a problem hiding this comment.
I've rolled this back so the only changes in test snapshots are because of additional tests and not change in existing tests.
42be855 to
a3216de
Compare
This branch has not been deployed
2 participants
Added
svyrep.designsupport across survey, statistical test, CI, and emmeans methods, allowing these functions to work with replicate-weight designs created bysurvey::svrepdesign()orsurvey::as.svrepdesign(). Previously, S3 dispatch and explicit class checks rejected these designs even though the underlying survey functions already supported them. Statistics continue to use the samesurveyimplementations, so variance estimates correctly account for replicate weights.construct_model()also now supportssvyrep.design, and emmeans methods useinherits(data, c("survey.design", "svyrep.design"))to correctly identify both survey design classes. Additionally,tbl_svyproduced from the {srvyr} package are also supported becausetbl_svyobjects are also of classsurvey.designorsvyrep.design. (#355, @szimmer)Closes #355
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()Reviewer Checklist (if item does not apply, mark is as complete)
devtools::test_coverage()When the branch is ready to be merged:
NEWS.mdwith the changes from this pull request under the heading "# cardx (development version)". If there is an issue associated with the pull request, reference it in parentheses at the end update (seeNEWS.mdfor examples).