PR for: #189 pre and post coefficients excluding omitted ones

[Constantino-Carreto-Romero]

Closes #189

[jorpppp]

@Constantino-Carreto-Romero With the example data:

xtevent y , panelvar(i) timevar(t) policyvar(z) window(5) impute(stag) diffavg proxy(x)

returns a "option diffavg not allowed" error message.

I figure there's some weird issue going on with the flow of the code because

xtevent y , panelvar(i) timevar(t) policyvar(z) window(5) norm(-1 -2) diffavg

returns a "norm not allowed" error message, when it should probably return a syntax error instead.

@Constantino-Carreto-Romero let's wait until I can see if I find more bugs here to tackle this. Maybe I'll tackle it while you work on other tasks.

[jorpppp]

Note that diffavg will return different results with trend(, method(ols)) and trend, method(gmm) because the coefficients for negative event time will differ between the two methods. I think this is fine but it is not properly explained in the helpfile and could lead to confusion. I opened #192 to address this later.

[jorpppp]

Note that diffavg will return different results with trend(, method(ols)) and trend, method(gmm) because the coefficients for negative event time will differ between the two methods. I think this is fine but it is not properly explained in the helpfile and could lead to confusion. I opened #192 to address this later.

Actually now I am thinking that we should exclude the endpoints from the diffavg calculation when we use trend. We omit them from the plot anyway. The endpoint is responsible for discrepancies in diffavg when using the two trend methods:

xtevent y, pol(z) window(5) trend(-2, method(ols)) diffavg
xtevent y, pol(z) window(5) trend(-2, method(gmm)) diffavg

Excluding the endpoints for this calculation would be consistent with #166

What do you think @Constantino-Carreto-Romero ?

[jorpppp]

@Constantino-Carreto-Romero With the example data:

xtevent y , panelvar(i) timevar(t) policyvar(z) window(5) impute(stag) diffavg proxy(x)

returns a "option diffavg not allowed" error message.

In 8d1f449 (#190) I enabled diffavg in _eventiv, so now diffavg can work with proxy()

[jorpppp]

I figure there's some weird issue going on with the flow of the code because

xtevent y , panelvar(i) timevar(t) policyvar(z) window(5) norm(-1 -2) diffavg

returns a "norm not allowed" error message, when it should probably return a syntax error instead.

@Constantino-Carreto-Romero let's wait until I can see if I find more bugs here to tackle this. Maybe I'll tackle it while you work on other tasks.

I actually found a deeper issue here. We use * in the syntax to pass options to the regression commands without actually spelling them out. This has a negative unintended consequence of skipping syntax checks. Consider the following example:

cap program drop mytest

program define mytest

syntax, [norm(integer -1 ) *]

di "`norm'"
di "`options'"

end

mytest, norm(-1)
mytest, norm(abc)

This returns:

which is unexpected. Because of the * in the syntax, Stata is not returning an invalid syntax error for mytest, norm(abc) even though it is not the right syntax because norm should be a string. Instead, it is using the default value for norm and passing the incorrectly-syntaxed norm(abc) option as an extra option in options.

This means that Stata is not properly checking for syntax errors in some places. This has not generated problems for us so far, but it might. I'll open a new issue for this.

Edit: We'll follow up on this in #193

[jorpppp]

Note that diffavg will return different results with trend(, method(ols)) and trend, method(gmm) because the coefficients for negative event time will differ between the two methods. I think this is fine but it is not properly explained in the helpfile and could lead to confusion. I opened #192 to address this later.

Actually now I am thinking that we should exclude the endpoints from the diffavg calculation when we use trend. We omit them from the plot anyway. The endpoint is responsible for discrepancies in diffavg when using the two trend methods:

xtevent y, pol(z) window(5) trend(-2, method(ols)) diffavg
xtevent y, pol(z) window(5) trend(-2, method(gmm)) diffavg

Excluding the endpoints for this calculation would be consistent with #166

What do you think @Constantino-Carreto-Romero ?

@Constantino-Carreto-Romero I'm going to go ahead and exclude the endpoints from the diffavg calculation when trend is active.

[jorpppp]

All looks good here, merging the PR now @Constantino-Carreto-Romero