Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Difference of averages doesn't include all coefficients #189

Closed
Constantino-Carreto-Romero opened this issue May 4, 2024 · 4 comments · Fixed by #190
Closed

Difference of averages doesn't include all coefficients #189

Constantino-Carreto-Romero opened this issue May 4, 2024 · 4 comments · Fixed by #190
Assignees
@Constantino-Carreto-Romero
Labels
bug Something isn't working

Comments

@Constantino-Carreto-Romero
Copy link
Collaborator

Per comment #178 (comment)

When executing

use example31, clear
xtevent y eta, panelvar(i) timevar(t) pol(z) impute(stag) window(max) diffavg

There is a bug because we get a calculated left endpoint of -19, but the computation of the difference of averages only includes until coefficient -9.
@Constantino-Carreto-Romero Constantino-Carreto-Romero added the bug Something isn't working label May 4, 2024
@Constantino-Carreto-Romero
Copy link
Collaborator Author

Constantino-Carreto-Romero commented May 7, 2024 •
edited
Loading

@jorpppp
The section code of diffavg is currently placed on this line in _eventols

xtevent/xtevent/_eventols.ado

Line 583 in 77b42a8

* DiD estimate

just before the code to adjust for a trend by GMM. Therefore, diffavg doesn't reflect the adjustment, if any. I'm moving the diffavg code to this line:

xtevent/xtevent/_eventols.ado

Line 681 in 77b42a8


after the trend adjustment code.

Constantino-Carreto-Romero added a commit that referenced this issue May 7, 2024
@Constantino-Carreto-Romero
#189 pre and post coefficients excluding omitted ones
42d88ea
@Constantino-Carreto-Romero
Copy link
Collaborator Author

@jorpppp
in 42d88ea I changed how diffavg selects the pre and post coefficients, so it doesn't rely on string matching. Instead, it relies on the local that indicates the omitted coefficients.
For instance, when running:

xtevent y eta, panelvar(i) timevar(t) pol(z) impute(stag) window(max) trend(-2, method(ols))  diffavg

I get the following output, which includes all event-time coefficients except for coefficients -2 and -1.

@jorpppp
Copy link
Collaborator

jorpppp commented May 7, 2024

Thanks @Constantino-Carreto-Romero , looks good. Can you start a PR for this please?

Merged
@Constantino-Carreto-Romero Constantino-Carreto-Romero linked a pull request May 7, 2024 that will close this issue
PR for: #189 pre and post coefficients excluding omitted ones #190
Merged
@Constantino-Carreto-Romero
Copy link
Collaborator Author

Summary: in this issue we fixed a bug in diffavg which was missing event-time coefficients in the computation of the difference of averages.
Thread continues in #190

jorpppp added a commit that referenced this issue May 8, 2024
@jorpppp
#189 #190 enable diff avg in _eventiv
8d1f449
jorpppp added a commit that referenced this issue May 9, 2024
@jorpppp
#189 #190 exclude endpoints from diffavg when trend is active
e103374
jorpppp added a commit that referenced this issue May 9, 2024
@Constantino-Carreto-Romero @jorpppp
PR for: #189 pre and post coefficients excluding omitted ones (#190)
fa0611b 
* #189 pre and post coefficients excluding omitted ones

* #189 #190 enable diff avg in _eventiv

* #189 #190 exclude endpoints from diffavg when trend is active

---------

Co-authored-by: jorpppp <jorpppp@gmail.com>
@jorpppp jorpppp mentioned this issue May 9, 2024
Merged
jorpppp added a commit that referenced this issue May 9, 2024
@jorpppp @Constantino-Carreto-Romero
PR for: #189 pre and post coefficients excluding omitted ones (#190) (#…
6cd3d5c 
…195)

* #189 pre and post coefficients excluding omitted ones

* #189 #190 enable diff avg in _eventiv

* #189 #190 exclude endpoints from diffavg when trend is active

---------

Co-authored-by: Constantino Carreto Romero <carretoromeroc@gmail.com>
jorpppp added a commit that referenced this issue May 9, 2024
@jorpppp @Constantino-Carreto-Romero
188 left window of zero returns an error (#194)
b9abc0b 
* #188 ordering when there are not pre-event dummies

* #188 ordering the left endpoint

* #188 Do not test for pretrends for plot if left window is zero

* #188 #194 Disable smoothest path if left window

* PR for: #189 pre and post coefficients excluding omitted ones (#190) (#195)

* #189 pre and post coefficients excluding omitted ones

* #189 #190 enable diff avg in _eventiv

* #189 #190 exclude endpoints from diffavg when trend is active

---------

Co-authored-by: Constantino Carreto Romero <carretoromeroc@gmail.com>

* #188 #194 Fix error messages for diffavg when windows are zero

* #188 #194 Fix lead selection and normalization when left window is zero or when many coefs are normalized

* #188 #194 Recheck that lead instrument is in estimation window when it is changed because normalizations coincide

* #188 #194 Do not test linear pretrend if too few pre-event coefs

---------

Co-authored-by: Constantino Carreto Romero <carretoromeroc@gmail.com>
jorpppp added a commit that referenced this issue May 14, 2024
@jorpppp @Constantino-Carreto-Romero
Bring #177 up to date again (#198)
50b9efc 
* #181 Fix check for constant treatment value and add returns (#182)

* 186 error when normalizing 0 coefficient (#187)

* #186 Fix bug when normalizing to 0 or positive

* #186 Propagate the fix to _eventiv

* PR for #170: let the window option choose a window range (#178)

* #170 bring changes from main

* #170 implement finding max or balanced window

* #170 examples to test implementation

* #170 add examples to test file

* #170 change window description and add example in help file

* #170 wording of calculated-window message

* #170 modify description of left and right window

* #170 wording of window limits

* #170 wording of calculated-window message

* #170 check for zero words in window

* #170 rearrange parsewindow checks

* #170 wording of window-parser error messges

* #170 add to help file example about maximum window

* #170 conflict with static

* "#170 revert previous commit"

This reverts commit c6d98aa.

* #170 parse window only when it is non-empty

* #170 window in cmdline parser

* #170 Delete issue170 special directory

* #170 wording of string window message

* #170 let max or balanced only if impute is specified

* #170 wording of error message

* #170 restructure checks for staggered adoption

* #170 restructure for IV case

* #170 informative window message and error message for balanced

* "#170 revert previous commit"

This reverts commit fc197a9.

* #170 informative window message and error message for balanced

* #170 paragraph for max balanced

* #170 check trend outside window for max and balanced

* #170 remove comment

* #170 trend specification if window is max or balanced

* #170 clear message about treatment time

* #170 check leads in proxyiv are in estimation window

* #170 wording of message about calculated window

* #178 Minor change to calculated window message

* #170 mark non-missing obs in varlist

* #170 examples to test implementation

* #170 pass marker and revert syntax of eventgenvars

* #170 Add marked sample to variables to keep when repeatedcs

* #170 Delete issue170 directory

* #178 Edits to helpfile

---------

Co-authored-by: jorpppp <jorpppp@gmail.com>

* PR for #179 Check behavior of the cohort and control_cohort options and allow automatic generation (#185)

* #179 Create cohort_consistency.do

* #179 Add consistency checks between cohort, control_cohort, and policyvar, add basic cohort variable creation

* #179 Change cohort, control_cohort to string in xtevent

* #179 Add bin and no reversion returns to _eventgenvars plus changes from #181

* #179 Enable saving created cohort variable

* #179 Automatic generation of control_cohort

* #179 Update help file

* #179 Modify test file

* #179 Delete issue folder

* #179 #185 Generate cohort in sample that ignores missings in control vars

* #179 #185 Add checks for existing cohort variables

* #179 #185 SA and proxy not allowed

* #179 #185 Fix bug in naming saved interaction variables

* #179 #185 Fix sorting bug when creating cohorts in repeatedcs setting

* PR for: #189 pre and post coefficients excluding omitted ones (#190)

* #189 pre and post coefficients excluding omitted ones

* #189 #190 enable diff avg in _eventiv

* #189 #190 exclude endpoints from diffavg when trend is active

---------

Co-authored-by: jorpppp <jorpppp@gmail.com>

* 188 left window of zero returns an error (#194)

* #188 ordering when there are not pre-event dummies

* #188 ordering the left endpoint

* #188 Do not test for pretrends for plot if left window is zero

* #188 #194 Disable smoothest path if left window

* PR for: #189 pre and post coefficients excluding omitted ones (#190) (#195)

* #189 pre and post coefficients excluding omitted ones

* #189 #190 enable diff avg in _eventiv

* #189 #190 exclude endpoints from diffavg when trend is active

---------

Co-authored-by: Constantino Carreto Romero <carretoromeroc@gmail.com>

* #188 #194 Fix error messages for diffavg when windows are zero

* #188 #194 Fix lead selection and normalization when left window is zero or when many coefs are normalized

* #188 #194 Recheck that lead instrument is in estimation window when it is changed because normalizations coincide

* #188 #194 Do not test linear pretrend if too few pre-event coefs

---------

Co-authored-by: Constantino Carreto Romero <carretoromeroc@gmail.com>

---------

Co-authored-by: Constantino Carreto Romero <carretoromeroc@gmail.com>
This was referenced May 14, 2024
Closed
Merged
jorpppp added a commit that referenced this issue May 14, 2024
@jorpppp @Constantino-Carreto-Romero
#177 allow replacing (#200)
f52a75b 
* Bring #177 up to date again (#198)

* #181 Fix check for constant treatment value and add returns (#182)

* 186 error when normalizing 0 coefficient (#187)

* #186 Fix bug when normalizing to 0 or positive

* #186 Propagate the fix to _eventiv

* PR for #170: let the window option choose a window range (#178)

* #170 bring changes from main

* #170 implement finding max or balanced window

* #170 examples to test implementation

* #170 add examples to test file

* #170 change window description and add example in help file

* #170 wording of calculated-window message

* #170 modify description of left and right window

* #170 wording of window limits

* #170 wording of calculated-window message

* #170 check for zero words in window

* #170 rearrange parsewindow checks

* #170 wording of window-parser error messges

* #170 add to help file example about maximum window

* #170 conflict with static

* "#170 revert previous commit"

This reverts commit c6d98aa.

* #170 parse window only when it is non-empty

* #170 window in cmdline parser

* #170 Delete issue170 special directory

* #170 wording of string window message

* #170 let max or balanced only if impute is specified

* #170 wording of error message

* #170 restructure checks for staggered adoption

* #170 restructure for IV case

* #170 informative window message and error message for balanced

* "#170 revert previous commit"

This reverts commit fc197a9.

* #170 informative window message and error message for balanced

* #170 paragraph for max balanced

* #170 check trend outside window for max and balanced

* #170 remove comment

* #170 trend specification if window is max or balanced

* #170 clear message about treatment time

* #170 check leads in proxyiv are in estimation window

* #170 wording of message about calculated window

* #178 Minor change to calculated window message

* #170 mark non-missing obs in varlist

* #170 examples to test implementation

* #170 pass marker and revert syntax of eventgenvars

* #170 Add marked sample to variables to keep when repeatedcs

* #170 Delete issue170 directory

* #178 Edits to helpfile

---------

Co-authored-by: jorpppp <jorpppp@gmail.com>

* PR for #179 Check behavior of the cohort and control_cohort options and allow automatic generation (#185)

* #179 Create cohort_consistency.do

* #179 Add consistency checks between cohort, control_cohort, and policyvar, add basic cohort variable creation

* #179 Change cohort, control_cohort to string in xtevent

* #179 Add bin and no reversion returns to _eventgenvars plus changes from #181

* #179 Enable saving created cohort variable

* #179 Automatic generation of control_cohort

* #179 Update help file

* #179 Modify test file

* #179 Delete issue folder

* #179 #185 Generate cohort in sample that ignores missings in control vars

* #179 #185 Add checks for existing cohort variables

* #179 #185 SA and proxy not allowed

* #179 #185 Fix bug in naming saved interaction variables

* #179 #185 Fix sorting bug when creating cohorts in repeatedcs setting

* PR for: #189 pre and post coefficients excluding omitted ones (#190)

* #189 pre and post coefficients excluding omitted ones

* #189 #190 enable diff avg in _eventiv

* #189 #190 exclude endpoints from diffavg when trend is active

---------

Co-authored-by: jorpppp <jorpppp@gmail.com>

* 188 left window of zero returns an error (#194)

* #188 ordering when there are not pre-event dummies

* #188 ordering the left endpoint

* #188 Do not test for pretrends for plot if left window is zero

* #188 #194 Disable smoothest path if left window

* PR for: #189 pre and post coefficients excluding omitted ones (#190) (#195)

* #189 pre and post coefficients excluding omitted ones

* #189 #190 enable diff avg in _eventiv

* #189 #190 exclude endpoints from diffavg when trend is active

---------

Co-authored-by: Constantino Carreto Romero <carretoromeroc@gmail.com>

* #188 #194 Fix error messages for diffavg when windows are zero

* #188 #194 Fix lead selection and normalization when left window is zero or when many coefs are normalized

* #188 #194 Recheck that lead instrument is in estimation window when it is changed because normalizations coincide

* #188 #194 Do not test linear pretrend if too few pre-event coefs

---------

Co-authored-by: Constantino Carreto Romero <carretoromeroc@gmail.com>

---------

Co-authored-by: Constantino Carreto Romero <carretoromeroc@gmail.com>

* Delete issue177 directory

* Revert "Delete issue177 directory"

This reverts commit 5c18688.

* Revert "Bring #177 up to date again (#198)"

This reverts commit 50b9efc.

---------

Co-authored-by: Constantino Carreto Romero <carretoromeroc@gmail.com>
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Constantino-Carreto-Romero Constantino-Carreto-Romero

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

PR for: #189 pre and post coefficients excluding omitted ones JMSLab/xtevent
2 participants
@jorpppp @Constantino-Carreto-Romero