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

Constantino-Carreto-Romero · 2024-04-14T19:52:52Z

Closes #170

This reverts commit c6d98aa.

xtevent/xtevent.ado

xtevent/_eventgenvars.ado

xtevent/_eventiv.ado

xtevent/xteventplot.ado

xtevent/xtevent.sthlp

Constantino-Carreto-Romero · 2024-05-03T16:30:25Z

@Constantino-Carreto-Romero If I run xtevent y eta, panelvar(i) timevar(t) policyvar(z) w(balanced) impute(stag) I get an error message. The found window limits are -1 and -1.
This doesn't seem right. If we run
xtevent y eta, panelvar(i) timevar(t) policyvar(z) w(balanced) impute(stag) savek(event)
bys i: egen min = min(event_evtime)
bys i: egen max = max(event_evtime)
preserve
bys i: keep if _n == 1
tab min
tab max
restore 
We get that all treated units have at least two periods pretreatment and one period post treatment (the treatment period, 0). So the balanced window should be -2, 0. Why is it not the case? Am I misreading how the balanced window should be calculated?
@jorpppp The calculated window is (-1 -1) plus the endpoints (-2+, 0+). The calculated limits are indeed (-2 0) but I follow the definition used for a numeric input for window and separate the periods covered by window and the endpoints. After the changes in 267cef7, running the same example produces this more informative error message.
@Constantino-Carreto-Romero Thanks. The error message is still a bit confusing. Maybe write "the calculated window is (-1,-1), excluding the endpoints". The -2+, 0+ is confusing, I don't know what the plus signs mean. So I would omit it.

@jorpppp
I edited the message in 8409db7. Is it ok?

In testing this I found a possible bug: xtevent y, w(0 3) pol(z) impute(stag) returns an error with the example31 data. Can you check please?

I tested it and I got the error message. The error comes from this line

xtevent/xtevent/_eventgenvars.ado

Line 454 in 8409db7

    
           by `panelvar' (`timevar'): egen long `minp'=min(`timevar') if !missing(_k_eq_`plus'`absk')

where the temporary variable being analyzed was not defined because this code section

xtevent/xtevent/_eventgenvars.ado

Line 430 in 8409db7

if `klevel'<0 {

was not executed because the left window is 0. I'm not sure whether it is a bug. It is not related to the automatic-window implementation, can I open a separate issue to check it?

jorpppp · 2024-05-03T16:45:18Z

@Constantino-Carreto-Romero If I run xtevent y eta, panelvar(i) timevar(t) policyvar(z) w(balanced) impute(stag) I get an error message. The found window limits are -1 and -1.
This doesn't seem right. If we run
xtevent y eta, panelvar(i) timevar(t) policyvar(z) w(balanced) impute(stag) savek(event)
bys i: egen min = min(event_evtime)
bys i: egen max = max(event_evtime)
preserve
bys i: keep if _n == 1
tab min
tab max
restore 
We get that all treated units have at least two periods pretreatment and one period post treatment (the treatment period, 0). So the balanced window should be -2, 0. Why is it not the case? Am I misreading how the balanced window should be calculated?
@jorpppp The calculated window is (-1 -1) plus the endpoints (-2+, 0+). The calculated limits are indeed (-2 0) but I follow the definition used for a numeric input for window and separate the periods covered by window and the endpoints. After the changes in 267cef7, running the same example produces this more informative error message.
@Constantino-Carreto-Romero Thanks. The error message is still a bit confusing. Maybe write "the calculated window is (-1,-1), excluding the endpoints". The -2+, 0+ is confusing, I don't know what the plus signs mean. So I would omit it.
@jorpppp I edited the message in 8409db7. Is it ok?

@Constantino-Carreto-Romero let me make a minor change to it in a push. After that I'll test the code a bit more and if it's ok I'll merge the PR.

In testing this I found a possible bug: xtevent y, w(0 3) pol(z) impute(stag) returns an error with the example31 data. Can you check please?

I tested it and I got the error message. The error comes from this line

xtevent/xtevent/_eventgenvars.ado

Line 454 in 8409db7

by `panelvar' (`timevar'): egen long `minp'=min(`timevar') if !missing(_k_eq_`plus'`absk')

where the temporary variable being analyzed was not defined because this code section

xtevent/xtevent/_eventgenvars.ado

Line 430 in 8409db7

if `klevel'<0 {

was not executed because the left window is 0. I'm not sure whether it is a bug. It is not related to the automatic-window implementation, can I open a separate issue to check it?

@Constantino-Carreto-Romero Yes, please open a new issue for this.

jorpppp · 2024-05-03T17:09:40Z

@Constantino-Carreto-Romero We still have bugs in this. With the example31 data:

gen pois = rpoisson(5) in 1/200
xtevent y eta i.pois, panelvar(i) timevar(t) pol(z) window(max) impute(stag)

returns an error message. I presume touse is not being set somewhere and the calculated window is using the entire sample and not the touse sample. Note that if we pass a sample restriction to xtevent this works:

xtevent y eta i.pois if pois!=., panelvar(i) timevar(t) pol(z) window(max) impute(stag)

Can you check this please?

Constantino-Carreto-Romero · 2024-05-03T17:34:24Z

@Constantino-Carreto-Romero We still have bugs in this. With the example31 data:
gen pois = rpoisson(5) in 1/200
xtevent y eta i.pois, panelvar(i) timevar(t) pol(z) window(max) impute(stag)
returns an error message. I presume touse is not being set somewhere and the calculated window is using the entire sample and not the touse sample. Note that if we pass a sample restriction to xtevent this works:
xtevent y eta i.pois if pois!=., panelvar(i) timevar(t) pol(z) window(max) impute(stag)
Can you check this please?

@jorpppp
I tried running the same example but cannot replicate the error message. When running

use example31, clear
gen pois = rpoisson(5) in 1/200
xtevent y eta i.pois, panelvar(i) timevar(t) pol(z) window(max) impute(stag)

I get the following outcome.

jorpppp · 2024-05-03T18:34:48Z

@Constantino-Carreto-Romero We still have bugs in this. With the example31 data:
gen pois = rpoisson(5) in 1/200
xtevent y eta i.pois, panelvar(i) timevar(t) pol(z) window(max) impute(stag)
returns an error message. I presume touse is not being set somewhere and the calculated window is using the entire sample and not the touse sample. Note that if we pass a sample restriction to xtevent this works:
xtevent y eta i.pois if pois!=., panelvar(i) timevar(t) pol(z) window(max) impute(stag)
Can you check this please?
@jorpppp I tried running the same example but cannot replicate the error message. When running
use example31, clear
gen pois = rpoisson(5) in 1/200
xtevent y eta i.pois, panelvar(i) timevar(t) pol(z) window(max) impute(stag)
I get the following outcome.

@Constantino-Carreto-Romero did not get the error because he wasn't trying to plot. Strictly speaking the error message appears with

xtevent y eta i.pois, panelvar(i) timevar(t) pol(z) window(max) impute(stag) plot

In any case there is a bug because the calculated window is not taking the missing values of the control variables into account, but it should. The issue has to do with the way samples are marked throughout: _eventgenvars is working with a sample that has not been marked for missing values of the variables, which is convenient for other reasons, e.g. imputation. @Constantino-Carreto-Romero will check this.

Constantino-Carreto-Romero · 2024-05-03T22:00:10Z

@Constantino-Carreto-Romero We still have bugs in this. With the example31 data:
gen pois = rpoisson(5) in 1/200
xtevent y eta i.pois, panelvar(i) timevar(t) pol(z) window(max) impute(stag)
returns an error message. I presume touse is not being set somewhere and the calculated window is using the entire sample and not the touse sample. Note that if we pass a sample restriction to xtevent this works:
xtevent y eta i.pois if pois!=., panelvar(i) timevar(t) pol(z) window(max) impute(stag)
Can you check this please?
@jorpppp I tried running the same example but cannot replicate the error message. When running
use example31, clear
gen pois = rpoisson(5) in 1/200
xtevent y eta i.pois, panelvar(i) timevar(t) pol(z) window(max) impute(stag)
I get the following outcome.
@Constantino-Carreto-Romero did not get the error because he wasn't trying to plot. Strictly speaking the error message appears with
xtevent y eta i.pois, panelvar(i) timevar(t) pol(z) window(max) impute(stag) plot
In any case there is a bug because the calculated window is not taking the missing values of the control variables into account, but it should. The issue has to do with the way samples are marked throughout: _eventgenvars is working with a sample that has not been marked for missing values of the variables, which is convenient for other reasons, e.g. imputation. @Constantino-Carreto-Romero will check this.

@jorpppp
in 6dcd33c I modified the syntax of _eventgenvars, so it can handle the local varlist. Then, I added a marksample to mark the non-missing observations in varlist and consider this restriction when calculating the window limits. This doesn't affect the already defined mark because the latter only considers if or in restrictions. I also updated the calls to _eventgenvars from other programs. I include in the do-file in the issue folder examples to test that:

the calculation of the window limits considers missing values in the local varlist
the consideration of if or in conditions were not affected

examples:

xtevent/issue170/issue170.do

Line 78 in 97dff85

** missing values in varlist

Constantino-Carreto-Romero · 2024-05-03T22:05:15Z

@jorpppp
in 97dff85 I uploaded an issue folder with a do-file with examples to test: i) error messages; ii) correct functionality; iii) interaction with other options and iv; that the implementation doesn't alter other functionalities.

jorpppp · 2024-05-03T22:27:06Z

@Constantino-Carreto-Romero We still have bugs in this. With the example31 data:
gen pois = rpoisson(5) in 1/200
xtevent y eta i.pois, panelvar(i) timevar(t) pol(z) window(max) impute(stag)
returns an error message. I presume touse is not being set somewhere and the calculated window is using the entire sample and not the touse sample. Note that if we pass a sample restriction to xtevent this works:
xtevent y eta i.pois if pois!=., panelvar(i) timevar(t) pol(z) window(max) impute(stag)
Can you check this please?
@jorpppp I tried running the same example but cannot replicate the error message. When running
use example31, clear
gen pois = rpoisson(5) in 1/200
xtevent y eta i.pois, panelvar(i) timevar(t) pol(z) window(max) impute(stag)
I get the following outcome.
@Constantino-Carreto-Romero did not get the error because he wasn't trying to plot. Strictly speaking the error message appears with
xtevent y eta i.pois, panelvar(i) timevar(t) pol(z) window(max) impute(stag) plot
In any case there is a bug because the calculated window is not taking the missing values of the control variables into account, but it should. The issue has to do with the way samples are marked throughout: _eventgenvars is working with a sample that has not been marked for missing values of the variables, which is convenient for other reasons, e.g. imputation. @Constantino-Carreto-Romero will check this.
@jorpppp in 6dcd33c I modified the syntax of _eventgenvars, so it can handle the local varlist. Then, I added a marksample to mark the non-missing observations in varlist and consider this restriction when calculating the window limits. This doesn't affect the already defined mark because the latter only considers if or in restrictions. I also updated the calls to _eventgenvars from other programs. I include in the do-file in the issue folder examples to test that:

the calculation of the window limits considers missing values in the local varlist

the consideration of if or in conditions were not affected

examples:

xtevent/issue170/issue170.do

Line 78 in 97dff85

** missing values in varlist

Thanks @Constantino-Carreto-Romero . This approach works but has a slight disadvantage. _eventgenvars is recalculating the marked sample that considers missing values in the variables. But this marked sample was already calculated by _eventols or _eventiv.

So instead of passing the varlist to _eventgenvars to repeat the marking, we can just pass the marked sample variable from _eventols or _eventiv to _eventgenvars as an option.

Constantino-Carreto-Romero · 2024-05-03T23:13:11Z

Thanks @Constantino-Carreto-Romero . This approach works but has a slight disadvantage. _eventgenvars is recalculating the marked sample that considers missing values in the variables. But this marked sample was already calculated by _eventols or _eventiv.

So instead of passing the varlist to _eventgenvars to repeat the marking, we can just pass the marked sample variable from _eventols or _eventiv to _eventgenvars as an option.

@jorpppp
thanks. In d7ae4e6 I reverted the changes to _eventgenvars's syntax and instead of, I passed the marked sample variable from _eventols, _eventiv, etc. as an option in _eventgenvars. Then, I ran the do-file in the issue folder to check that this change worked.

Constantino-Carreto-Romero · 2024-05-03T23:59:44Z

When running

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 differences of averages only includes until coefficient -9.

…ps://github.com/JMSLab/xtevent into 170-let-the-window-option-choose-a-window-range

* #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>

* 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>

Constantino-Carreto-Romero added 17 commits March 26, 2024 20:03

#170 bring changes from main

d6790a5

#170 implement finding max or balanced window

480ba3c

#170 examples to test implementation

746bf80

#170 add examples to test file

9892674

#170 change window description and add example in help file

70cc357

#170 wording of calculated-window message

9a043b6

#170 modify description of left and right window

3160407

#170 wording of window limits

ffb1ef6

#170 wording of calculated-window message

3de513b

#170 check for zero words in window

9396e7b

#170 rearrange parsewindow checks

9d15508

#170 wording of window-parser error messges

090456e

#170 add to help file example about maximum window

7c67683

#170 conflict with static

c6d98aa

"#170 revert previous commit"

1a91dad

This reverts commit c6d98aa.

#170 parse window only when it is non-empty

4017f25

#170 window in cmdline parser

80e8a07

Constantino-Carreto-Romero requested a review from jorpppp April 14, 2024 19:52

Constantino-Carreto-Romero self-assigned this Apr 14, 2024

Constantino-Carreto-Romero linked an issue Apr 14, 2024 that may be closed by this pull request

Let the window option choose a window range based on event-time limits in the data #170

Closed

Constantino-Carreto-Romero added 2 commits April 14, 2024 13:59

Merge branch 'main' into 170-let-the-window-option-choose-a-window-range

a6a4e2a

#170 Delete issue170 special directory

320c5cc