survey_prop should default to proportions #142
Conversation
Codecov Report
@@ Coverage Diff @@
## main #142 +/- ##
==========================================
- Coverage 83.27% 83.19% -0.08%
==========================================
Files 22 22
Lines 1106 1113 +7
==========================================
+ Hits 921 926 +5
- Misses 185 187 +2
Continue to review full report at Codecov.
|
|
Updated with messaging that default behavior has changed. While I was at it, I added another method to estimate confidence intervals. |
| proportion = proportion, prop_method = prop_method, | ||
| deff = deff, df = df, .svy = cur_svy())) | ||
| if (missing(x)){ | ||
| if (missing(proportion) & ("ci" %in% vartype)){ |
There was a problem hiding this comment.
This message is not actually true because of how missing args works (even though (missing(proportion) is TRUE, the default value comes from line 97 not line 143 in this branch of code).
I actually kind of prefer this behavior - my argument would be that this way we're better adhering to the mental model that survey_mean's behavior comes from survey::svymean and survey_prop's comes from survey::svyciprop. But wanted to check if you disagreed.
My preference is to fix by removing this inform, but the other option is to change line 97 so that it TRUE.
library(srvyr)
#>
#> Attaching package: 'srvyr'
#> The following object is masked from 'package:stats':
#>
#> filter
data(api, package = "survey")
dstrata <- apistrat %>%
as_survey_design(strata = stype, weights = pw)
dstrata %>%
group_by(above = api99 > 600) %>%
summarise(
prop_default = survey_mean(vartype = "ci"),
prop_true = survey_mean(proportion = TRUE, vartype = "ci"),
prop_false = survey_mean(proportion = FALSE, vartype = "ci")
) %>% select(ends_with("low"))
#> When `proportion` is unspecified, `survey_mean()` now defaults to `proportion = TRUE` when `x` is left out. This should improve confidence interval coverage.
#> This message is displayed once per session.
#> # A tibble: 2 × 3
#> prop_default_low prop_true_low prop_false_low
#> <dbl> <dbl> <dbl>
#> 1 0.365 0.367 0.365
#> 2 0.483 0.483 0.483There was a problem hiding this comment.
Sorry, just noticed this as I was getting ready to submit. I need to submit in the next few days, so plan to go with my gut when I get back to this later tonight, unless I hear from you
| deff = FALSE, | ||
| df = NULL, | ||
| ... | ||
| ) { | ||
| .svy <- cur_svy() | ||
| .full_svy <- cur_svy_full() | ||
|
|
||
| if (missing(proportion) & ("ci" %in% vartype)){ | ||
| inform("When `proportion` is unspecified, `survey_prop()` now defaults to `proportion = TRUE`. This should improve confidence interval coverage.", |
There was a problem hiding this comment.
After seeing how long the message is, let's break it up by doing the following:
inform(
c(
"When `proportion` is unspecified, `survey_prop()` now defaults to `proportion = TRUE`.",
i = "This should improve confidence interval coverage."
),
.frequency = "once", .frequency_id="spd"
)
This seems like a more logical default given the context people will expect. Documentation already states:
This addresses issue #141