Closed frederickluser closed 1 year ago
Dear Grant,
I implemented all your requested changes and ran the check function locally on my computer. I received no errors, warnings, or notes.
Additionally, I added the option to run a hypothesis test for the heterogeneous effects in the emfx
file. Maybe you like this option.
Super, thanks Frederik. I appreciate you bearing with me and think we're close now. I have three more requests and then will merge after a final check:
inst/tinytest
. I'm fine for you to use simulated data if that's easiest. Ideally, however, I want to make sure that we're benchmarking against a known set of results, since I obviously don't want to push through new changes that might give misleading results to users.xvar
functionality---e.g., in the vignette---please do say.LMK if anything is unclear, or if you'd like to discuss any of these requests. Thanks!
Thank you for your feedback, Grant. Regarding your points:
I implemented the tinytest
. I called them test_emfx_xvar.R
and test_etwfe_xvar.R
and used in both the mpdta
dataset so far.
To test the xvar
argument, I added a random binary or categorical variable (xvar
also works with more than two values, while continuous variable make less sense. Nonetheless, the code also runs for continuous variables).
Finally, I ran again a devtools::check
that looks fine.
I could not find so far a nice dataset. I tested so far the mpdta
as well as data("pj_officer_level_balanced", package = "staggered")
(large, with 500k observations) as well as simulated data and the results are as expected.
I also ran the function against the did
package and a standard TWFE. I attach my file where I do these comparisons:
etwfe_comparisons.zip
I added myself in the description.
Final point - I found a way to get rid of the [1]
index in emfx. I think it's less error-prone like this.
This is great. I have a couple tweaks that I'd like to make on the backend, but happy to merge as-is. Many thanks for the nice contribution!
HU that emfx
now autodetects whether to estimate het. TEs based on whether xvar
is detected as an attribute of the underlying emfx object. Users can override this behavior with the new "by_xvar" argument. See #21.
very nice!
A related thought - if the user enters a continuous xvar (which is fine), marginaleffects should evaluate at a meaningful value of xvar, e.g., the mean.
You think this might be something so think about?
A related thought - if the user enters a continuous xvar (which is fine), marginaleffects should evaluate at a meaningful value of xvar, e.g., the mean.
Sounds like a good idea. But I'm not sure I'll have the bandwidth to incorporate before the next CRAN submission. I'm hoping to do today or tomorrow, since the current version is error'ing out as a result of the upstream marginaleffects 0.9.0 changes (fixed in this dev version). But please feel free to raise another issue/PR if you have an implementation plan.
Dear Grant,
I implemented now the heterogeneous treatment effects I mentioned in issue #13.
Interacting the treatment with a time-constant covariate allows estimating the effect separately for multiple groups, for example, women and men (see the image below).
I use this now in my research, and maybe you find it interesting to include in your package.
All the best, Frederic