Conversation
change _eventols and _eventiv to create _eq_m1 var
There was a problem hiding this comment.
Pull request overview
This PR modifies the handling of the normalized/omitted event-time variable (e.g., _eq_m1) when using the kvars() option in event study estimations. The key change is to allow the _eq_<kvomit> variable to be created during the loop iteration, but then remove it afterward, rather than skipping it during the loop.
Key changes:
- Modified the loop logic to no longer skip
_eq_<kvomit>variables during iteration - Added post-loop removal of
_k_eq_<kvomit>from both theincludedandnamesvariable lists - Added
quiprefix toclonevarcommand in_eventiv.adofor consistency
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| xtevent/_eventols.ado | Updated kvars handling to create and then remove the _eq_<kvomit> variable instead of skipping it |
| xtevent/_eventiv.ado | Same kvars handling update as _eventols.ado, plus added qui prefix to clonevar for quiet execution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| loc remove "_k_eq_`kvomit'" | ||
| loc included : list local included - remove | ||
| loc names : subinstr local names `""_k_eq_`kvomit'".."' "" |
There was a problem hiding this comment.
The string substitution pattern will only correctly remove _k_eq_<kvomit> from the names list if it's the first element. The pattern ""_k_eq_kvomit'".."'looks for the variable name followed by the..` separator, but:
- If the variable is in the middle, the pattern would be
.."k_eq"..which won't match - If it's the last element, it won't have
..after it, so it won't match
Consider using a more robust approach to handle all positions, such as additional substitution patterns to cover .."k_eqkvomit'""(middle/end case) or using the list manipulation that's already being done for theincluded` local on line 245.
| loc names : subinstr local names `""_k_eq_`kvomit'".."' "" | |
| * Rebuild `names` from the filtered `included` list to safely omit `_k_eq_`kvomit'` | |
| loc names "" | |
| foreach sub of local included { | |
| if "`names'" == "" loc names `""`sub'""' | |
| else loc names `"`names'.."`sub'""' | |
| } |
| if "`komittrend'"=="." loc komittrend = "" | ||
| loc remove "_k_eq_`kvomit'" | ||
| loc included : list local included - remove | ||
| loc names : subinstr local names `""_k_eq_`kvomit'".."' "" |
There was a problem hiding this comment.
The string substitution pattern will only correctly remove _k_eq_<kvomit> from the names list if it's the first element. The pattern ""_k_eq_kvomit'".."'looks for the variable name followed by the..` separator, but:
- If the variable is in the middle, the pattern would be
.."k_eq"..which won't match - If it's the last element, it won't have
..after it, so it won't match
Consider using a more robust approach to handle all positions, such as additional substitution patterns to cover .."k_eqkvomit'""(middle/end case) or using the list manipulation that's already being done for theincluded` local on line 359.
| loc names : subinstr local names `""_k_eq_`kvomit'".."' "" | |
| * Rebuild names from the updated included list to ensure _k_eq_`kvomit' is removed in all positions | |
| loc names "" | |
| loc j = 1 | |
| foreach sub of local included { | |
| if `j' == 1 loc names `""`sub'""' | |
| else loc names `"`names'.."`sub'""' | |
| loc ++j | |
| } |
| foreach var of varlist `kvstub'* { | ||
| if `norm' < 0 loc kvomit = "m`=abs(`norm')'" | ||
| else loc kvomit "p`=abs(`norm')'" |
There was a problem hiding this comment.
The kvomit variable is being recalculated on every iteration of the loop, but since norm doesn't change during iteration, the value will be the same each time. This is inefficient. Consider moving the calculation outside the loop, before line 227, to compute it once.
| foreach var of varlist `kvstub'* { | |
| if `norm' < 0 loc kvomit = "m`=abs(`norm')'" | |
| else loc kvomit "p`=abs(`norm')'" | |
| if `norm' < 0 loc kvomit = "m`=abs(`norm')'" | |
| else loc kvomit "p`=abs(`norm')'" | |
| foreach var of varlist `kvstub'* { |
| foreach var of varlist `kvstub'* { | ||
| if `norm' < 0 loc kvomit = "m`=abs(`norm')'" | ||
| else loc kvomit "p`=abs(`norm')'" |
There was a problem hiding this comment.
The kvomit variable is being recalculated on every iteration of the loop, but since norm doesn't change during iteration, the value will be the same each time. This is inefficient. Consider moving the calculation outside the loop, before line 339, to compute it once.
| foreach var of varlist `kvstub'* { | |
| if `norm' < 0 loc kvomit = "m`=abs(`norm')'" | |
| else loc kvomit "p`=abs(`norm')'" | |
| if `norm' < 0 loc kvomit = "m`=abs(`norm')'" | |
| else loc kvomit "p`=abs(`norm')'" | |
| foreach var of varlist `kvstub'* { |
|
I need to check these Copilot suggestions, we'll then check for errors again and merge the PR @Constantino-Carreto-Romero @knerii |
1 participant
change _eventols and _eventiv to create _eq_m1 var