search

JMSLab / xtevent

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

#203 repeated use restricting to e(sample) leads to different results #205

Closed jorpppp closed 3 months ago

jorpppp commented 3 months ago

Closes #203

jorpppp commented 3 months ago

@Constantino-Carreto-Romero to check this, can you please run the test file with the code in the main branch, store the log file, then run the test file with the code here, store the log file, and then compare the log files using Notepad++ or another text editor? I'm 99% sure that changing the e(sample) as we did won't change anything but I'd like to be sure.

Constantino-Carreto-Romero commented 3 months ago

@Constantino-Carreto-Romero to check this, can you please run the test file with the code in the main branch, store the log file, then run the test file with the code here, store the log file, and then compare the log files using Notepad++ or another text editor? I'm 99% sure that changing the e(sample) as we did won't change anything but I'd like to be sure.

@jorpppp This is a comparison of the test file from main (left side) and the test file from #203 (right side). There some differences in the files. Some of them are because of the usage of generated random variables without a seed, so I think there is no problem, for instance, https://github.com/JMSLab/xtevent/blob/13c9fefd95ca38935a1fc914834b860dbbdf5fb8/test/test.do#L110 or https://github.com/JMSLab/xtevent/blob/13c9fefd95ca38935a1fc914834b860dbbdf5fb8/test/test.do#L429 or due to a different naming of the temporary variables, for instance in the output from the execution of https://github.com/JMSLab/xtevent/blob/13c9fefd95ca38935a1fc914834b860dbbdf5fb8/test/test.do#L268 Nonetheless, there are other differences to look at. They come from the execution of the overlay plot examples. For instance in

https://github.com/JMSLab/xtevent/blob/13c9fefd95ca38935a1fc914834b860dbbdf5fb8/test/test.do#L242 the differences look like this.

jorpppp commented 3 months ago

Thanks @Constantino-Carreto-Romero (the online diffchecker is cool).

The diference with the overlay(static) graphs is arising because the static model that is estimated for the plot is restricted to the estimation sample. Since now the estimation sample is bigger, the static model uses a larger sample.

In principle there is no reason why the static model should be restricted to the sample where the leads and lags are defined, as in this case, so the new behavior should be fine.

I am going to add a seed to the test file to avoid random differences showing up in the diffs later on, then I'll merge the PR.