Update difference in difference docs to show TWFE formulation #482
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #482 +/- ##
=======================================
Coverage 94.40% 94.40%
=======================================
Files 29 29
Lines 2075 2075
=======================================
Hits 1959 1959
Misses 116 116 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
drbenvincent
changed the title
|
View / edit / reply to this conversation on ReviewNB ostojanovic commented on 2025-06-05T18:34:18Z I'm not sure if this is not rendering properly in my browser, but I still see drbenvincent commented on 2025-06-06T09:54:14Z Where did you see that? In a local build of the docs? In the remote build on this PR it renders ok https://causalpy--482.org.readthedocs.build/en/482/notebooks/did_pymc_banks.html and on my local build of the docs. You might need to run this? pip install 'causalpy[docs]'
EDIT: Oh, yes if you were looking at reviewnb that does not render the notebook properly, it's relatively crude and doesn't render the citations and references like you get when running it through sphinx ostojanovic commented on 2025-06-06T17:14:35Z Yes, it's the ReviewNB issue. |
|
View / edit / reply to this conversation on ReviewNB ostojanovic commented on 2025-06-05T18:34:19Z Is this the original paper: https://www.journals.uchicago.edu/doi/10.1086/649603 ? I know you cite the book, but maybe we should also have a link to the original paper? Feel free to do it however you see fit. drbenvincent commented on 2025-06-06T09:57:19Z Yep, the existing reference does cite that paper. It also appears in the reference section. So I think there's something wrong with whatever you were looking at? |
|
View / edit / reply to this conversation on ReviewNB ostojanovic commented on 2025-06-05T18:34:20Z there's a typo in baseline drbenvincent commented on 2025-06-06T10:00:37Z thanks |
|
View / edit / reply to this conversation on ReviewNB ostojanovic commented on 2025-06-05T18:34:20Z I think you should have output here, it would make sense for a reader to see the pairplot drbenvincent commented on 2025-06-06T10:24:13Z Originally I opted to not show this because it's a bit of a side show and will be fixed in the near future. But now I've displayed the pair plot, but have it collapsed by default, but users will be able to click to view. (viewable only in a proper render of the docs, not here on reviewnb) |
|
View / edit / reply to this conversation on ReviewNB ostojanovic commented on 2025-06-05T18:34:21Z Btw, I see these deprecation warnings throughout the notebook. I think we should either suppress them or write a sentence or two on why they happen, so that someone reading the documentation knows why we're ignoring them. drbenvincent commented on 2025-06-06T10:28:03Z Sure thing. Running the notebook again just now, those DeprecationWarning's have disappeared. There was a remaining FutureWarning which I've suppressed. |
|
View / edit / reply to this conversation on ReviewNB ostojanovic commented on 2025-06-05T18:34:22Z Is the text supposed to be this large? drbenvincent commented on 2025-06-06T10:29:09Z This is some stupid rendering of reviewnb - it's not like this in local or remote builds of the docs |
|
View / edit / reply to this conversation on ReviewNB ostojanovic commented on 2025-06-05T18:34:23Z I think it would be good to have one or two more sentences on what the two-way fixed effects (TWFE) formulation is, maybe with a link to an article or a paper? Or maybe we have more documentation on it here, in
Again, I'm not sure if it's just not rendering properly for me, but I still see the formula as drbenvincent commented on 2025-06-06T11:08:34Z Same thing about the formatting and reviewnb.
Good idea - I've added a bite more info and some references. We will likely revisit this topic in more detail (either in this notebook, or in more general docs information) because the DID literature is vast.
|
|
View / edit / reply to this conversation on ReviewNB ostojanovic commented on 2025-06-05T18:34:24Z There's nothing in references. I'm not sure whether you prefer to have links for each term or a bibliography at the end. Either way, it's fine by me, but then maybe you can just delete the headline here if it's not needed. drbenvincent commented on 2025-06-06T10:35:48Z This will be reviewnb again.
|
There was a problem hiding this comment.
@drbenvincent This looks good! I have made a few comments, mostly formatting and text. Tell me what you think, and then it's ready to go.
|
Where did you see that? In a local build of the docs? In the remote build on this PR it renders ok https://causalpy--482.org.readthedocs.build/en/482/notebooks/did_pymc_banks.html
View entire conversation on ReviewNB |
|
Yep, the existing reference does cite that paper. It also appears in the reference section. So I think there's something wrong with whatever you were looking at? View entire conversation on ReviewNB |
|
thanks View entire conversation on ReviewNB |
|
Originally I opted to not show this because it's a bit of a side show and will be fixed in the near future. But now I've displayed the pair plot, but have it collapsed by default, but users will be able to click to view. (viewable only in a proper render of the docs, not here on reviewnb) View entire conversation on ReviewNB |
|
Sure thing. Running the notebook again just now, those DeprecationWarning's have disappeared. There was a remaining FutureWarning which I've suppressed. View entire conversation on ReviewNB |
|
This is some stupid rendering of reviewnb - it's not like this in local or remote builds of the docs View entire conversation on ReviewNB |
|
This will be reviewnb again.
View entire conversation on ReviewNB |
|
Same thing about the formatting and reviewnb.
Good idea - I've added a bite more info and some references. We will likely revisit this topic in more detail (either in this notebook, or in more general docs information) because the DID literature is vast.
View entire conversation on ReviewNB |
|
Thanks for the review @ostojanovic. Many of the issues related to formatting - let me just clarify about that. ReviewNB gives some kind of rendering of the ReviewNB is kind of useful, but when evaluating formatting issues I would take a look at the remote build of the docs. We've got a bot which auto adds a link to remote docs in the initial PR description. Look for the 📚 Documentation preview 📚 bit. |
|
Yes, it's the ReviewNB issue. View entire conversation on ReviewNB |
Previously there were 2 models presented. The first was a classic 2x2 fit on just the years immediately pre and post intervention. The second was a fit to all the years worth of data, but the model also added a linear trend term.
We now have a new model (model 2) squeezed in between. This corresponds to the 2x2 DID model but with all years worth of data included. This is also what they show in Mastering Metrics (page 188).
Model 4 is also new and is the two way fixed effects formulation of DID.
NOTE: The fit of model 1 is still problematic - it has divergences. I think this is most likely because the scale of the data is quite far off what is assumed by the (built-in) priors in the linear regression pymc model. I will come back to this once we've got the ability to add custom priors (see #387), and this might actually happen sooner rather than later. So the plan is to come back and fix Model 1 and expand the summary discussion which compares the models.
📚 Documentation preview 📚: https://causalpy--482.org.readthedocs.build/en/482/