gphk-metrics / stata-multe

Multiple Treatment Effects
17 stars 4 forks source link

Pull Request for #8: Package Speedup #9

Closed mcaceresb closed 2 years ago

mcaceresb commented 2 years ago

@jerrayc I've moved the contamination bias decomposition to be computed separately from the main estimation. This speeds up the function run significantly as this took the bulk of the runtime. Please check:

jerrayc commented 2 years ago

@mcaceresb Reviewing this PR and making edits/comments now. A few initial things to flag:

. use test/example_star.dta

. multe score treatment, control(school)
(MulTE() in lmulte, compiled by Stata 17.0, is too new to be run by this version of
Stata and so was ignored)
                 <istmt>:  3499  MulTE() not found
r(3499);
mcaceresb commented 2 years ago

@jerrayc I rebuilt the mata file with 14.1. Should be good. Let's make it a task in this PR to make sure the repo makes sense if it should be public. This includes:

I'll work on the above and update the top level tasks when I do.

jerrayc commented 2 years ago

@mcaceresb Does it make sense to make a new branch that is meant to be public, or should we somehow store parts of this repo away that we might not want to be part of the repo (i.e. some of the files in test)?

Or is there a way to set up the repo to keep some files private while others are public?

The other thing is if we want to eventually submit this to SSC, then we would want to save the SSC submission file somewhere until we are ready to submit it.

mcaceresb commented 2 years ago

@mcaceresb Does it make sense to make a new branch that is meant to be public, or should we somehow store parts of this repo away that we might not want to be part of the repo (i.e. some of the files in test)?

Or is there a way to set up the repo to keep some files private while others are public?

The other thing is if we want to eventually submit this to SSC, then we would want to save the SSC submission file somewhere until we are ready to submit it.

@jerrayc It is the nature of git that all is public, as it goes. We can, of course, "clean" the main branch and leave the rest in a side-branch, but this would all be public in the end (though perhaps obfuscated). I believe it should be possible to keep the repository private and make a public website for it. This might be something to look into after we wrap this up.

Either way when we merge this, we should open an issue called "Taking the package public" just to get confirmation from the team, or do you think that's overkill? We can also ask in that issue whether it's OK to make the whole repo public or if they'd prefer parts remain private. If the latter, I think a website might be the solution (though I haven't tried the private/public thing before, it's supposed to be doable).

jerrayc commented 2 years ago

@mcaceresb Besides the open question about what to do with the timer code, and besides the preparation for public repo (last two top level tasks), I have confirmed the rest of the top level tasks and reviewed all code changes.

jerrayc commented 2 years ago

@mcaceresb Update re making repo public: I talked to Peter and we agreed to make it public, and that it's fine to leave the extraneous files (e.g. SSC submission text, test do files, etc.). I think a reasonable course of action would be:

  1. Make the file public for the initial NBER draft release
  2. Make a separate branch to work on SSC submission
  3. Once the SSC submission is ready, merge to main, submit the package to SSC, and make the repo private again.

Does this sound good to you?

mcaceresb commented 2 years ago

@jerrayc I have never switched a private-gone-public repo back to private, but it looks simple enough. I should note that clones and forks of the repository will not be undone.

I have deleted references to the timer. To be clear:

LMK.

jerrayc commented 2 years ago

@mcaceresb Yes, that is what I mean re: bullet points.

Sounds fine if forks and clones will not be undone -- I don't imagine many of those will exist anyway.

Sounds good re: removing timer references.

Ready to squash and merge when you are!

mcaceresb commented 2 years ago

Summary here.