Skip to content

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

Merged
jorpppp merged 10 commits into
mainfrom
217-check-results-reposting-for-sun-and-abraham-implementation
May 30, 2024
Merged

#217 Fix results reposting for Sun and Abraham implementation#219
jorpppp merged 10 commits into
mainfrom
217-check-results-reposting-for-sun-and-abraham-implementation

Conversation

@jorpppp
Copy link
Copy Markdown
Collaborator

@jorpppp jorpppp commented May 28, 2024

Closes #217

jorpppp and others added 5 commits May 23, 2024 16:23
* #210 Changes to xtevent helpfile, enable abbreviations

* #210 Changes to other helpfiles

* #214 Change ci default style to p1 (#215) (#216)

* #210 add link to impute note

* #210 minor edit

* #210 minor edit to acknowledgements

* #210 minor clarification

* #210 update description of impute

* "#210 revert previous commit"

This reverts commit 824b616.

* #210 update impute note

* #210 revert minor unintended changes

* #210 docs to create impute note

* #210 #213 Delete issue folder

---------

Co-authored-by: Constantino Carreto Romero <carretoromeroc@gmail.com>
@jorpppp
Copy link
Copy Markdown
Collaborator Author

jorpppp commented May 28, 2024

@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:

  • 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.
  • We do not want to drop the other coefficients from the interacted regression away. If you look at how eventstudyinteract works, it only outputs the aggregated event-time coefs in the coefficient table, but it actually keeps the entire e(b) and e(V) from the interacted regression, and saves the aggregates in e(b_interact) and e(V_interact). We want to keep the entire interacted regression coefs and variance as well because we may be interested in the coefficients from the interacted regression. We are saving e(b) and e(V) but these do not seem to include the coefficients on eta in the above example.

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

@Constantino-Carreto-Romero
Copy link
Copy Markdown
Contributor

  • 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 bddc29b 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 49fbce9 where there are no changes in outputs (some changes in the format of some messages).

@jorpppp
Copy link
Copy Markdown
Collaborator Author

jorpppp commented May 29, 2024

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
Copy link
Copy Markdown
Contributor

Constantino-Carreto-Romero commented May 30, 2024

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 a3de100 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 bfc7cb9 I updated the help file (also changed the order of some related outputs).

@jorpppp
Copy link
Copy Markdown
Collaborator Author

jorpppp commented May 30, 2024

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

@Constantino-Carreto-Romero
Copy link
Copy Markdown
Contributor

@jorpppp
In c0dee2a I updated the test file's log. There were not changes.

@jorpppp
Copy link
Copy Markdown
Collaborator Author

jorpppp commented May 30, 2024

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

@jorpppp jorpppp merged commit e308958 into main May 30, 2024
@jorpppp jorpppp deleted the 217-check-results-reposting-for-sun-and-abraham-implementation branch May 30, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Check results reposting for Sun and Abraham implementation

2 participants