ietoolkit icon indicating copy to clipboard operation
ietoolkit copied to clipboard

[iedorep][ieboilstart] iedorep wipes out postfiles when the input dofile contains `clear all` in Stata 16

Open luisesanmartin opened this issue 2 years ago • 10 comments

If a dofile containing the command clear all is used as input for iedorep, that command erases the postfiles generated by iedorep where it stores the reproducibility check results.

This, for example:

clear all
set obs 10
gen var1 = runiform()

Generates this error:

. iedorep my-dofile.do
 
Processing: my-dofile.do
post posty not found
r(111);

While this other one doesn't generate an error:

clear
set obs 10
gen var1 = runiform()

Output:

. iedorep my-dofile.do
 
Processing: my-dofile.do

  +------------------------------------------------------+
  | Line |           Data |        Seed | Sort | Subfile |
  |------+----------------+-------------+------+---------|
  |    3 | ERROR! Changed | ERROR! Used |      |         |
  +------------------------------------------------------+

This problem occurs in Stata 16 but not in Stata 17, which doesn't erase postfiles when clear all is used

luisesanmartin avatar Feb 09 '22 22:02 luisesanmartin

This issue also explains #268. I'm closing that issue since the error is better explained here

luisesanmartin avatar Feb 09 '22 22:02 luisesanmartin

@kbjarkefur This will break when using ieboilstart for the same reason. I fixed by copying out everything that clear says it does and removing postutil clear and discard: https://github.com/worldbank/ietoolkit/blob/4a4bc25184f6e47e09284497b1e8331d42cf9f82/src/ado_files/iedorep.ado#L274-L290

bbdaniels avatar Feb 10 '22 20:02 bbdaniels

Not sure what task I am assigned to do but I do not have time before iebaltab is out anyways

kbjarkefur avatar Feb 14 '22 08:02 kbjarkefur

Not sure what task I am assigned to do but I do not have time before iebaltab is out anyways

Not a problem -- it is a reminder to check whether ieboilstart and iedorep function together, and to update ieboilstart in case they do not. This is on hold till after iebaltab anyways since it needs feedback and testing from you before moving forward and that is a much higher priority!

bbdaniels avatar Feb 15 '22 18:02 bbdaniels

Not sure I agree with this being a solution to the actual problem. clear all is frequently used in Stata code. Changing the code in ieboilstart can be done, but this could break backward compatibility if someone using already published versions of ieboilstart and count on the clear all behavior. And since changing ieboilstart does not fix how iedorep behaves when clear all is used in other cases, I am not sure this is the right trade-off.

kbjarkefur avatar Feb 21 '22 10:02 kbjarkefur

I understand that -- I am leaning towards a solution where if clear all or the equivalent is detected in a target do-file, it is simply replaced by the above in the iedorep version of the file. There are very few places where that would affect real outcomes of code except in highly specialized cases, which I am willing to accept with appropriate documentation (ie, we can return a warning that clear all was replaced). But having it be fundamentally incompatible with code from our own package that we expect people to use is, obviously, not acceptable.

Rather than altering ieboilstart defaults, I can envision that it would be possible for ieboilstart to have an (undocumented) option allowing this substitution, which iedorep would add to the end of any ieboilstart call. Since they are in the same package and this is not a ton of work we should agree on some solution to it, and that could work perfectly here without affecting any other ieboilstart functionality or compatibility. I am happy to implement but since you know the command best your technical advice and support is essential! Again, no rush on this since it's targeted for around June but it would be good for me to know our options here. Thanks!

bbdaniels avatar Feb 22 '22 17:02 bbdaniels

Ok, If we can make this work, which seems possible, then I think this is the best solution. I can take a stab at the ieboilstart part of this when I have more time.

kbjarkefur avatar Feb 22 '22 19:02 kbjarkefur

Yeah don't rush on it. I think it is not hard at all on that side, I can probably implement them myself since it's just a new option and a corresponding if/else statement at the below line. Let me know if anything else would have to change to make it work.

https://github.com/worldbank/ietoolkit/blob/880cc03175e0edb98a6e7c01b1113851a81150be/src/ado_files/ieboilstart.ado#L193-L194

bbdaniels avatar Feb 22 '22 20:02 bbdaniels

I agree that it seems as if the solution is only that. If you want, gimme a PR and I can review in more detail and think a bit deeper about it.

kbjarkefur avatar Feb 24 '22 09:02 kbjarkefur

Ok, I think I was able to do it in 932ec9764d886bfb1752bf6e09dd955f9c5ec1c5 without changing ieboilstart -- instead of altering internal code, I manually implemented the clear commands and then use ieboilstart's existing noclear option. This seems to work as intended, and I don't think it will affect any other functionality in most use cases. Let me know if you see any potential issues here. Thanks!

bbdaniels avatar Mar 02 '22 15:03 bbdaniels