JMSLab / xtevent

Stata package -xtevent-
MIT License
43 stars 12 forks source link

Plot several models from xtevent in one plot with xteventplot #7

Open izmartinez opened 2 years ago

izmartinez commented 2 years ago

It would be useful if one could plot several event study models into one plot using xteventplot.

Currently, one can in principle resort to the user-written command coefplot. However, for this it would be useful if there was an estimate _k_eq_m1 saved for t-1 (and similarly, all the other pre-treatment time points when adding the trend(#) option to the estimation) in the coefficient vector e(b).

jorpppp commented 2 years ago

@rayhuang11 will look into this. The first step is to produce a event study plot with two coefficient paths next to each other, from two coefficient and variance matrices. Then, we can look into doing the same from xtevent output.

Also look at the other DID commands out there, you can find them in the Asjad Naqvi guide. At least one of those commands has syntax for a multiple event study plot that we may be able to use.

rayhuang11 commented 2 years ago

Update: did not do any work on this in the past week.

rayhuang11 commented 2 years ago

@Constantino-Carreto-Romero @jorpppp I may have found a potential bug. When I run

xtevent y eta , panelvar(i) timevar(t) policyvar(z) window(5) plot

on the example.dta and try to get the x-axis values for the plot via e(mattrendx), I get the error r(111) matrix e(mattrendx) not found. The same applies to e(mattrendy).

rayhuang11 commented 2 years ago

The first step is to produce a event study plot with two coefficient paths next to each other, from two coefficient and variance matrices. Then, we can look into doing the same from xtevent output.

Update: successfully created some fake data produced an event study plot with two coefficients paths next to each other from the matrices taken from the xtevent command.

For future reference: the link for of Asjad Naqvi's guide on DiD is here.

jorpppp commented 2 years ago

@Constantino-Carreto-Romero @jorpppp I may have found a potential bug. When I run

xtevent y eta , panelvar(i) timevar(t) policyvar(z) window(5) plot

on the example.dta and try to get the x-axis values for the plot via e(mattrendx), I get the error r(111) matrix e(mattrendx) not found. The same applies to e(mattrendy).

This is not a bug, just lack of clarity in the documentation. We'll edit the documentation in #71.

jorpppp commented 2 years ago

We are thinking that to make this work, we need to create a command that makes repeated calls to xteventplot to generate the x and ya values to be plotted from multiple model estimates. The first step to implement this is to add an offset option to xteventplot that offsets the x-values of the event coefficients by a fixed amount.

@rayhuang11 will check Asjad Naqvi's guide to see if there's another easier way to do this. If not, he'll start by adding the offset option to xteventplot.

rayhuang11 commented 2 years ago

@rayhuang11 will check Asjad Naqvi's guide to see if there's another easier way to do this.

@jorpppp I did not find anything in Naqvi's guide.

jorpppp commented 2 years ago

Thanks @rayhuang11, we'll implement the offset option then.

jorpppp commented 2 years ago

@rayhuang11 will also check the eventdd package

rayhuang11 commented 2 years ago

No updates from @rayhuang11 for this week.

jorpppp commented 2 years ago

Per this comment, there's a function in the did_imputation package that we may want to look at:

https://github.com/JMSLab/xtevent/issues/35#issuecomment-1206498863

rayhuang11 commented 2 years ago

Per this comment, there's a function in the did_imputation package that we may want to look at: https://github.com/JMSLab/xtevent/issues/35#issuecomment-1206498863

The did_imputation package uses event_plot.ado to plot up to 8 models in the same graph. Here is the example from their repo: five_estimators_example

The offset here is using the default distance which is not user specified. Rather, the distance depends on how many models are being plotted together. I've imitated a version of their offset command in https://github.com/JMSLab/xtevent/commit/386fd82c4aedf3e6b458b8f01b9737345ffeead3 to the issue7_RH_test.do file. @jorpppp do we prefer this version or the version where the offset is manually specified by the user as we initially discussed (e.g. offset(0.2) would offset each model by 0.2)?

rayhuang11 commented 2 years ago

Current approach:

Taking inspiration from event_plot.ado from the did_imputation package, we can adapt xteventplot so that when we use xteventplot the way we have been doing so all along, everything stays the same (i.e. calling xtevent then xteventplot.

However, if the user specifies xteventplot model1 model2 model3..., options xteventplot will graph the several models together. The user can access the saved results from the different models through the estimates store command. Then xteventplot will run the existing code in a loop for each model, then take the stored results and overall them on top of one another. The main code we would be adding would be an offset option.

My main concerns at this point would be how all the options would interact with this approach. E.g. if we used the option noci, we would force all the models to not display confidence intervals.

Note: we would have to pick a max number of models to allow. Currently, did_imputation allows for up to 8.

I also uploaded event_plot.ado here.

Fyi @jorpppp @Constantino-Carreto-Romero.

rayhuang11 commented 2 years ago

@Constantino-Carreto-Romero thanks for the help yesterday!

For today's meeting, we should go over my plan to adapt xteventplot.ado to allow for the plotting of multiple models in https://github.com/JMSLab/xtevent/commit/762be427653d800e34ccfd88d4ddd443d7a89d3c.

What options will be incompatible with multiple models? e.g. I don't think we should allow p-values to be displayed.

izmartinez commented 2 years ago

Thank you all for your work and service to the research community!

I'm working on a new project where we'll use event studies, too, and I'm already looking forward to these new features you're adding.

jorpppp commented 2 years ago

@Constantino-Carreto-Romero thanks for the help yesterday!

For today's meeting, we should go over my plan to adapt xteventplot.ado to allow for the plotting of multiple models in 762be42.

What options will be incompatible with multiple models? e.g. I don't think we should allow p-values to be displayed.

@rayhuang11 I think the pseudocode here is the proper way to go. Some thoughts about how to handle xteventplot options per call today:

rayhuang11 commented 2 years ago

Status update

Thanks @Constantino-Carreto-Romero for the help!

As of https://github.com/JMSLab/xtevent/commit/20fc0374af3643a4fa17b62c2f6ed87b7d692e89, xteventplot can now plot multiple models with the offset. Currently, we require that the normalized coefficient be the same for all the models.

All the disabled options requested in https://github.com/JMSLab/xtevent/issues/7#issuecomment-1213340711 have been disabled. Options that should apply to all models are work as intended.

  • Options noci, nosupt, scatterplotopts, suptplotopts, ciplotopts, and suptreps should work as repeated options, as in the following syntax: xteventplot model1 model2, ci(ci noci) supt(supt nosupt) scatterplotopts(mcolor(red green))

This part is almost working as intended. There is a small bug that I have yet to address involving tokenize and local positional macros. Namely, I call tokenize multiple times, and for the second time I call tokenize, the local macros in memory still refer to the first tokenize call. We can discuss this in tomorrow's meeting if I have yet to find a solution.

Fyi @jorpppp.

rayhuang11 commented 2 years ago

Things to discuss:

jorpppp commented 2 years ago

Thanks @rayhuang11. Per today's call:

jorpppp commented 2 years ago

There should be some entry about repeated options in the manual section for syntax

rayhuang11 commented 2 years ago

Final status update

For the final state of the code, please see the issue7_xteventplot_test.ado file under the issue_7 folder.

What works:

Not implemented:

Things to note:

As of the posting of this comment, I am not planning on doing any more work on this issue. Of course, feel free to reach out to me if anything seems unclear. Thanks!

jorpppp commented 2 years ago

Thank you @rayhuang11 ! There's been a lot of progress in this issue. @Constantino-Carreto-Romero and I can finish this up. We'll let you know if there are any questions.

jorpppp commented 1 year ago

Pending, @jorpppp has to check.

jorpppp commented 1 year ago

Pending, @jorpppp has to check.

jorpppp commented 1 year ago

I managed to do some basic testing of the modified xteventplot and I glanced at the code. There are a few things that we should fix before tackling the options that are listed as not implemented in https://github.com/JMSLab/xtevent/issues/7#issuecomment-1239905659

I'll try to work on these a bit, next week we can revisit with @Constantino-Carreto-Romero to pass some of these tasks to him.

jorpppp commented 1 year ago

The branch for this issue was brought to date in https://github.com/JMSLab/xtevent/commit/e326a4b47f164d660dbbbacb7399a64e48997f8e

jorpppp commented 1 year ago

@Constantino-Carreto-Romero will check this after #59 for Monday's meeting.

Constantino-Carreto-Romero commented 1 year ago

Update: I have already started to revise this issue.

Constantino-Carreto-Romero commented 1 year ago
  • [ ] The default offset (0.2) seems small.
  • [ ] We should be able to have different offsets between models when there are multiple models

Regarding those points: the default offset of 0.2 looks ok with two or three models, but for more models, the graph looks cramped. Maybe we could change the default offset to be more flexible. I think a simple rule could be to set offset equal to 0.n - 0.1, where n is the number of models. I'm going to test that rule and bring some examples.

Constantino-Carreto-Romero commented 1 year ago

in https://github.com/JMSLab/xtevent/commit/79405d6ab316628f06dfdb430c6d756125387e4e I changed the default offset, so now the point estimates for a corresponding x-axis label are placed according to the following:

This was the previous rule for the offset. I changed it for this rule.

in https://github.com/JMSLab/xtevent/commit/cee873814d6a65c4cfa298fc5f84c7113a7ab049 I create plots with 3 models and with 6 models. These are the plots with the previous default offset: 3 models original, 6 models original. With the new rule for the default offset: 3 models, 6 models. Currently, the offset option works for two models only, and its input defines how much distance the second point estimate will be to the right of the first point estimate. I think we could modify this option as well.

Constantino-Carreto-Romero commented 1 year ago

@jorpppp

Constantino-Carreto-Romero commented 1 year ago

Per call:

jorpppp commented 1 year ago

@Constantino-Carreto-Romero Just checking to see if there's any progress on this.