Skip to content

#230 create _eq_m1 when using kvars( ) #231

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

Open
jorpppp wants to merge 1 commit into
base: main
Choose a base branch
from
Open

#230 create _eq_m1 when using kvars( ) #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions xtevent/_eventiv.ado
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,10 +339,10 @@ program define _eventiv, rclass
foreach var of varlist `kvstub'* {
if `norm' < 0 loc kvomit = "m`=abs(`norm')'"
else loc kvomit "p`=abs(`norm')'"
Comment on lines 339 to 341
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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'* {

Copilot uses AI. Check for mistakes.
if "`var'"=="`kvstub'_evtime" | "`var'" == "`kvstub'_eq_`kvomit'" continue
if "`var'"=="`kvstub'_evtime" continue
if "`kvstub'"!="_k" {
loc sub : subinstr local var "`kvstub'" "_k", all
clonevar `sub' = `var'
qui clonevar `sub' = `var'
}
else {
loc sub = "`var'"
Expand All @@ -354,7 +354,10 @@ program define _eventiv, rclass
loc ++ j
}
loc komittrend=r(komittrend)
if "`komittrend'"=="." loc komittrend = ""
if "`komittrend'"=="." loc komittrend = ""
loc remove "_k_eq_`kvomit'"
loc included : list local included - remove
loc names : subinstr local names `""_k_eq_`kvomit'".."' ""
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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
}

Copilot uses AI. Check for mistakes.
}
*"
loc komit "`norm' `komittrend'"
Expand Down
7 changes: 5 additions & 2 deletions xtevent/_eventols.ado
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ program define _eventols, rclass
foreach var of varlist `kvstub'* {
if `norm' < 0 loc kvomit = "m`=abs(`norm')'"
else loc kvomit "p`=abs(`norm')'"
Comment on lines 227 to 229
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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'* {

Copilot uses AI. Check for mistakes.
if "`var'"=="`kvstub'_evtime" | "`var'" == "`kvstub'_eq_`kvomit'" continue
if "`var'"=="`kvstub'_evtime" continue
if "`kvstub'"!="_k" {
loc sub : subinstr local var "`kvstub'" "_k", all
qui clonevar `sub' = `var'
Expand All @@ -240,7 +240,10 @@ program define _eventols, rclass
* "
loc included "`included' `sub'"
loc ++ j
}
}
loc remove "_k_eq_`kvomit'"
loc included : list local included - remove
loc names : subinstr local names `""_k_eq_`kvomit'".."' ""
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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'""'
}

Copilot uses AI. Check for mistakes.
}
loc komit "`norm'`komittrend'"
loc komit = strtrim("`komit'")
Expand Down