DeclareDesign / DesignLibrary

Library of Research Designs
https://declaredesign.org/library/
Other
30 stars 3 forks source link

Move `tidy` to import from generics #207

Closed lukesonnet closed 5 years ago

lukesonnet commented 5 years ago

This PR changes the importing to only rely on getting tidy from generics and it removes the broom package from Suggests.

The main problem with this PR is that it will require estimatr >= 0.13.0 which won't be on GitHub until that branch (https://github.com/DeclareDesign/estimatr/pull/264) is merged in, hopefully today. So tests will fail for now.

That estimatr patch can go to CRAN ASAP, so this shouldn't hold up DesignLibrary.

jaspercooper commented 5 years ago

Thanks so much for this @lukesonnet, really appreciate it. No worries to have this breaking pending CRAN version being bumped to 13.

I do have something weird happening on this PR where I am still getting Error: No tidy method for objects of class lm_robust when running designs, even after installing estimatr from your generics branch.

Could it be that DD is using an old version of tidy estimator or something?

For example, this breaks: regression_discontinuity_designer().

But if you run the design code,

N <- 1000
 tau <- 0.15
 outcome_sd <- 0.1
 cutoff <- 0.5
 bandwidth <- 0.5
 control_coefs <- c(0.5, 0.5)
 treatment_coefs <- c(-5, 1)
 poly_reg_order <- 4

 # M: Model
 control <- function(X) {
   as.vector(poly(X, length(control_coefs), raw = T) %*% control_coefs)}
 treatment <- function(X) {
   as.vector(poly(X, length(treatment_coefs), raw = T) %*% treatment_coefs) + tau}
 population <- declare_population(
   N = N,
   X = runif(N,0,1) - cutoff,
   noise = rnorm(N,0,outcome_sd),
   Z = 1 * (X > 0))
 potential_outcomes <- declare_potential_outcomes(
   Y_Z_0 = control(X) + noise,
   Y_Z_1 = treatment(X) + noise)
 reveal_Y <- declare_reveal(Y)

 # I: Inquiry
 estimand <- declare_estimand(LATE = treatment(0) - control(0))

 # D: Data Strategy
 sampling <- declare_sampling(handler = function(data){
   subset(data,(X > 0 - abs(bandwidth)) & X < 0 + abs(bandwidth))})

 # A: Answer Strategy 
 estimator <- declare_estimator(
   formula = Y ~ poly(X, poly_reg_order) * Z,
   model = lm_robust,
   term = "Z",
   estimand = estimand)

 # Design
 regression_discontinuity_design <- 
   population + potential_outcomes + estimand + reveal_Y + sampling + estimator

This works as it should: tidy(lm_robust(Y ~ Z, draw_data(regression_discontinuity_design))) even though this does not: regression_discontinuity_design.

lukesonnet commented 5 years ago

Please install generics from CRAN and install the latest broom from GitHub. Then if it still doesn't work please post your sessionInfo().

I need to figure out whether this is compatible with having the old broom installed on your machine.

jaspercooper commented 5 years ago

That fixes it, thanks.

jaspercooper commented 5 years ago

So what does this imply for the designlibrary dependencies in description?

lukesonnet commented 5 years ago

This PR removes broom from Suggests, adds generics to Imports, and requires the right version of estimatr.

However, having the old broom loaded messes with tidy, so if anyone loads the old broom they'll have this problem. I'm not sure the best solution to this problem, @nfultz. Is there a way to demand the newer version of broom be loaded if they must have a version of broom? I could put in a workaround in estimatr that checks if broom is loaded and clobbers it, but that seems like the incorrect way to do this.

nfultz commented 5 years ago

If you are going to do anything in estimatr::.onLoad don't unload another package, cran will probably not accept that. I would warning message loudly that broom should be upgraded to a newer version I guess and that tidy() may not work otherwise.

nfultz commented 5 years ago

Also probably need to fix https://github.com/DeclareDesign/DeclareDesign/blob/master/R/declare_estimator.R#L304-L308 - that may be the source of some conflicts (although broom isn't being attached there, it calls the version of the base function directly), especially if people are seeing problems with non-estimatr model functions such as glm.

lukesonnet commented 5 years ago

Okay, so current plan is to add a warning in estimatr::.onLoad that checks whether the old version of broom is loaded and warns if yes.