asheshrambachan / HonestDiD

Robust inference in difference-in-differences and event study designs
Other
175 stars 45 forks source link

Updated readme with unified example #14

Closed jonathandroth closed 2 years ago

asheshrambachan commented 2 years ago

Thanks! This looks overall great to me.

Some small comments below based on the compiled readme over here.

  1. For some reason, \bar{M} is typeset oddly throughout (see screenshot). Perhaps \overline{M} will typeset better? Screen Shot 2022-09-14 at 4 18 06 PM
  2. Under ``robust confidence intervals,'' should it be: these confidence intervals account for the fact that there is estimation error both in the treatment effects estimates and our estimates of the pre-trends.
  3. At the beginning of the example usage section, we should say in text that we got this example from Scott Cunningham's Causal Mixtape book.
  4. Should we explicitly say in the text that we use the did package to estimate the event-study specification (with a link to the package.)? I also think in the code snippets, we should use the convention package_name::function so that its clear what functions come from which package. Obviously not necessary for utilities like here, dplyr, ggplot2 but would be good for did, fixest, and haven. Doing so would make it as clear as possible about what comes from where to someone who may not be super familiar with R. (although the stata package makes this less necessary).
  5. Under `Sensitivity analysis using relative magnitudes restrictions,' is it worth doing some more hand-holding through what each input to the function means? I think for numPrePeriods, numPostPeriods etc the comments in the snippet are sufficient, but l_vec may not be as obvious.
  6. It would be easier to read if you formatted functions using code text as opposed to italics -- e.g., see screenshot. Screen Shot 2022-09-14 at 4 29 42 PM
  7. The code snippet under "Sensitivity Analysis Using Smoothness Restrictions" doesn't input l_vec. Should we mention it specifies a default?
jonathandroth commented 2 years ago

Thanks for the comments!

Re 1), hmmm. Github only recently implemented Latex support and it's kinda buggy. The Mbar shows up fine when I compile locally. Unfortunately, \overline{M} does not appear to be supported in Markdown. As a hack, I replaced the Mbar equations with links to images of the equations generated by https://latex.codecogs.com/. I also replaced the TWFE equation in this way because it was displaying weird.

Re 2), yes, fixed.

Re 3) -- this is not from Scott's book. It is an example I came up with when I did a lecture as part of his "Mixtape sessions" lecture series. (Based on an example from Bacon, but I don't think we need to cite him.)

Re 4), I've added in explicit package references. We do say that we use the did package, and I added a link to it.

Re 5) and 7), I agree that the previous usage of l_vec was confusing. Since the default is to use the first period, I have removed l_vec from the examples you mentioned. I then added a more detailed explanation of l_vec in the section where we do average effects.

Re 6), I have fixed this.

I have also added a section at the end with a link to the old vignette that discusses additional features, and to a youtube video of a talk I gave about this.

On Wed, Sep 14, 2022 at 4:33 PM Ashesh Rambachan @.***> wrote:

Thanks! This looks overall great to me.

Some small comments below based on the compiled readme over here https://github.com/jonathandroth/HonestDiDCopy#honestdid.

  1. For some reason, \bar{M} is typeset oddly throughout (see screenshot). Perhaps \overline{M} will typeset better? [image: Screen Shot 2022-09-14 at 4 18 06 PM] https://user-images.githubusercontent.com/29416461/190253994-4da0dffd-fce5-4bfc-9010-aee2fdff91d4.png
  2. Under ``robust confidence intervals,'' should it be: these confidence intervals account for the fact that there is estimation error both in the treatment effects estimates and our estimates of the pre-trends.
  3. At the beginning of the example usage section, we should say in text that we got this example from Scott Cunningham's Causal Mixtape book.
  4. Should we explicitly say in the text that we use the did package to estimate the event-study specification (with a link to the package.)? I also think in the code snippets, we should use the convention package_name::function so that its clear what functions come from which package. Obviously not necessary for utilities like here, dplyr, ggplot2 but would be good for did, fixest, and haven. Doing so would make it as clear as possible about what comes from where to someone who may not be super familiar with R. (although the stata package makes this less necessary).
  5. Under `Sensitivity analysis using relative magnitudes restrictions,' is it worth doing some more hand-holding through what each input to the function means? I think for numPrePeriods, numPostPeriods etc the comments in the snippet are sufficient, but l_vec may not be as obvious.
  6. It would be easier to read if you formatted functions using code text as opposed to italics -- e.g., see screenshot. [image: Screen Shot 2022-09-14 at 4 29 42 PM] https://user-images.githubusercontent.com/29416461/190256257-45fd32e1-b5c7-4d50-b94b-600c2fe15a58.png
  7. The code snippet under "Sensitivity Analysis Using Smoothness Restrictions" doesn't input l_vec. Should we mention it specifies a default?

— Reply to this email directly, view it on GitHub https://github.com/asheshrambachan/HonestDiD/pull/14#issuecomment-1247272048, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE6EXFHHI5UIF4QV63B7LEDV6IZATANCNFSM6AAAAAAQMXKKCQ . You are receiving this because you authored the thread.Message ID: @.***>

asheshrambachan commented 2 years ago

Awesome!

Re 3) Ah got it. I saw the data was being pulled from the causal mixtape, so didn't know if that was an example in his book!

One final thing:

8) Under `Sensitivity Analysis Using Smoothness Restrictions,'' you added the examplel_vec = basisVector(3, numPostPeriods).Should we just switch it tobasisVector(2, numPostPeriods)` since there are only two post periods in the example?

Aside from that, these fixes look good to me and I'll merge in the pull request.

jonathandroth commented 2 years ago

Sounds good - I have updated that example. Think we're good to merge.