JMSLab / xtevent

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

#217 Fix results reposting for Sun and Abraham implementation #219

Closed jorpppp closed 2 months ago

jorpppp commented 2 months ago

Closes #217

jorpppp commented 2 months ago

@Constantino-Carreto-Romero The following returns an error:

xtevent y eta , policyvar(z) window(5) vce(cluster i) impute(stag) sunab trend(-3)

This error happens because the GMM trend code is trying to adjust the covariance between the other coefficients and the event-time coefficients after adjusting the latter by the trend. The reposting now clears all the other coefficients, so the GMM adjustment code does not find any other coefficients to adjust.

So I think two things need to be fixed:

Can you check this please @Constantino-Carreto-Romero ?

Constantino-Carreto-Romero commented 2 months ago
  • If we use Sun Abraham with a trend adjustment, there won't be other coefficients to adjust, and so the code needs to skip the adjustment of the covariance.

@jorpppp trend adjustment by GMM when there are no covariates could happen in settings other than SA, for instance when running

xtevent y , policyvar(z) window(5) vce(cluster i) impute(stag) nofe note noconstant trend(-3)

showed the same error message. Therefore, in https://github.com/JMSLab/xtevent/pull/219/commits/bddc29b437c2e9611b7f5f17f1b64da1f140e47d I implemented it, so the program first checks whether there are covariates or a constant in the the regression output. If there are not, the mata code skips adjusting by the covariance with convariates. This is the note for gmm trend extrapolation for reference. Running the following examples of SA without/with trend adjustment by GMM

xtevent y eta , policyvar(z) window(5) vce(cluster i) impute(stag) sunab
xtevent y eta , policyvar(z) window(5) vce(cluster i) impute(stag) sunab trend(-3)

produces 1 and 2. Then, I ran the test file and uploaded the log file in https://github.com/JMSLab/xtevent/pull/219/commits/49fbce98ae0253d39225fb6c2f93d783acf4f0c5 where there are no changes in outputs (some changes in the format of some messages).

jorpppp commented 2 months ago

Thanks @Constantino-Carreto-Romero, this looks good. We are only pending the check about saving the results of the interacted regression. I think e(b) and e(V) should save the aggregates, as we are doing now, such that -test- works on the aggregates, but we need to save the full results from the interacted regression too.

Constantino-Carreto-Romero commented 2 months ago

Thanks @Constantino-Carreto-Romero, this looks good. We are only pending the check about saving the results of the interacted regression. I think e(b) and e(V) should save the aggregates, as we are doing now, such that -test- works on the aggregates, but we need to save the full results from the interacted regression too.

@jorpppp in https://github.com/JMSLab/xtevent/pull/219/commits/a3de1002f0e2b2b9ba577f2a02bce12a6ead5ef0 I implemented it. The cohort-relative-time interaction variables for the interacted regression were created as temporary variables, so their names were not informative. I retrieved the informative names and used them to rename the row/column names of b and V from the interacted regression. Then, I renamed those matrices as b_ir and V_ir. Are these names ok? EventStudyInteract doesn't rename them and leave them as b and V, but we cannot use those names because they are reserved for the corrected coefficients and covariance matrix. When running the following example

xtevent y eta , policyvar(z) window(5) vce(cluster i) impute(stag) sunabraham reghdfe savek(aa, saveinteract replace)
mat li e(b_ir)

I get the following output. In https://github.com/JMSLab/xtevent/pull/219/commits/bfc7cb9876e267354b26a404596be602c19604d4 I updated the help file (also changed the order of some related outputs).

jorpppp commented 2 months ago

Thank you @Constantino-Carreto-Romero , this looks good. Did you run the test file again after this last change?

Constantino-Carreto-Romero commented 2 months ago

@jorpppp In https://github.com/JMSLab/xtevent/pull/219/commits/c0dee2a3953934dcb4517585a3c91a11cfce518e I updated the test file's log. There were not changes.

jorpppp commented 2 months ago

Thank you @Constantino-Carreto-Romero . Merging now.