Fixes weighting for lm_lin and blocked difference_in_means #169
Conversation
This file contains 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
nfultz
approved these changes
Mar 15, 2018
R/estimatr_lm_lin.R
Outdated
| if (is.numeric(weights)) { | ||
| center <- apply(demeaned_covars, 2, weighted.mean, weights) | ||
| } else { | ||
| center <- TRUE |
There was a problem hiding this comment.
center <- colMeans(...)
or simliar - generally don't set a variable to different types in different branches.
R/estimatr_lm_lin.R
Outdated
| ], | ||
| center = TRUE, | ||
| demeaned_covars, | ||
| center = center, |
There was a problem hiding this comment.
Given above, change scale to sweep.
tests/testthat/test-lm-lin.R
Outdated
| lmo$coefficients["am"] | ||
| ) | ||
|
|
||
| # Weighted matches (one covar) |
There was a problem hiding this comment.
split these into different test_that blocks with appropriate lables.
R/estimatr_difference_in_means.R
Outdated
| # rescale weights for convenience | ||
| if (is.numeric(model_data$weights)) { | ||
| weight_mean <- mean(model_data$weights) | ||
| data$weights <- model_data$weights / weight_mean |
There was a problem hiding this comment.
If weight_mean is never used again, ok to put all onto one line.
This was referenced Mar 17, 2018
Closed
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.
Previously,
lm_linwas not demeaning covariates by the weighted mean, and was just using the simple mean (Issue #148). This has been solved.After solving it, a test failed checking
difference_in_meansagainstlm_linin the blocked, weighted case. This test previously passed because both were wrong in the same say!The above problem is solved now. In
difference_in_means()we compute the post-stratification estimator as:The problem was that we used
NandN_overallrather than the sum of the weights in a block and the sum of all of the weights if you had a blocked, weighted specification. NowNandN_overallappropriately capture the weights.