feat: accept svyrep.design in the emmeans functions - #358
Open
szimmer wants to merge 1 commit into
Open
Conversation
`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>
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_emmeans_emmeans()andard_emmeans_contrast()now accept replicate-weight survey designs (svyrep.design), andconstruct_model()gained asvyrep.designmethod. Alongside the class check, both functions selected the data withdplyr::last(class(data)) == "survey.design", which does not identify asvyrep.design; the test is nowinherits(data, c("survey.design", "svyrep.design")). (Feature Request: Supportsvyrep.design(replicate-weight) survey objects #355, @szimmer)Provide more detail here as needed.
This is the last piece of #355, after #356 (the six summary
ard_*methods) and #357 (the survey test and CI functions). It is whatgtsummary::add_difference(test = "emmeans")requires on a replicate design; the defaultsvy.t.testpath is covered by #357.Checked end to end against
gtsummary, with #356 and #357 in place:add_difference(test = "emmeans"), linearizedadd_difference(test = "emmeans"), replicateemmeans::contrast()on the replicate modelThe replicate interval and p-value differ from the linearized design and match emmeans computed directly on the replicate model, so the replicate weights are genuinely carried through rather than silently ignored.
There were two problems here, not one. The class guard was the visible one. Underneath it, both functions selected the model data like this:
That happens to work for
survey.design2, whose last class element is"survey.design", but asvyrep.designfails the test entirely and would have fallen through to theelsebranch — passing the design object where a data frame is expected. Relaxing the guard alone would have produced a confusing downstream failure rather than support. The test is nowinherits(data, c("survey.design", "svyrep.design")), which is also less brittle forsurvey.designsubclasses generally.construct_model()needed asvyrep.designmethod for the same sibling-class reason. It delegates to thesurvey.designmethod; the model is fit bysurvey::svyglm(), which already handles replicate designs.emmeans itself needed 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:sch.widePoint estimates agree exactly across the two designs, as they must — the sampling weights are identical and only the variance estimation differs.
Tests (
tests/testthat/test-svyrep.design_emmeans.R) cover both functions,construct_model(), that the standard errors are replicate-based rather than a silent fallback to linearization, and adata.framecase confirming the changed class test did not alter the non-survey path.Reference GitHub issue associated with pull request.
Addresses #355. With #356 and #357, this completes it.
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:
construct_model.svyrep.design()delegates to thesurvey.designmethod, soset_cli_abort_call()and the package checks are inherited.lintrreports no findings on any changed line.