asheshrambachan / HonestDiD

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

Fix bug in RM functions with 1 period #50

Closed jonathandroth closed 9 months ago

jonathandroth commented 10 months ago

This PR addresses a bug when using DeltaRM with only 1 post-treatment period raised in #49. The issue appears to stem from the logic in constructing A here, which only restricts to the post-treatment moments when numPostPeriods > 1. The bug fix here applies the same logic to the case where numPostPeriods = 1 (although this requires slightly different syntax to avoid dimension issue). I made similar tweaks to the other RM functions.

After the bug fix, I get nearly identical answers using 1 post-period or 2 post-periods in the example given in #49.

@mcaceresb @asheshrambachan if this looks good to you I will merge. BTW, do we have an unit testing for these functions? I did not see any here.

mcaceresb commented 10 months ago

@jonathandroth While the if/else logic for postPeriodRows is analogous by default, one possible difference is if dropZero=FALSE is passed to .create_A_RM et al. (currently set up to never happen I believe). If this option is deprecated then it's fine; if it's not then you can double-check the logic for a single post-period is correct even with that option?

jonathandroth commented 10 months ago

Thanks! I think currently HonestDiD assumes that the coefficient for the omitted period is not included, and this is reflected by the fact that dropZero is always set to TRUE internally, and there does not appear to be an option for the user to set it to FALSE. At some point we might want to build out the option to include the zero coef, in which case we'd have to update this code block, but I think that's a low priority for right now.

mcaceresb commented 9 months ago

@jonathandroth I added some basic checks; at least all the various options do run.

And since you mentioned dropZero is always TRUE, this PR is good to merge.

jonathandroth commented 9 months ago

Thanks, Mauricio!