PR for #19: Compare smoothest path implementation with STATA's xtevent

[santiagohermo]

Closes #19. In this PR we want to review the modifications to the code that computes the smoothest path.

• In this comment I added a description of how the code is structured @SimonFreyaldenhoven. Let me know if you find any issues or have questions!
• Per @jmshapir comment I won't be adding @nateschor as reviewer.

Issue folders contain computations used for discussion in #19 and should be eliminated.

[SimonFreyaldenhoven]

@santiagohermo Thanks! I will try to go through the AddSmPath function before the end of the week!

[SimonFreyaldenhoven]

The code looks good to me, so I have nothing beyond a couple of very minor comments, expect for one question: Did we already try a different method than Nelder and Mead?

[SimonFreyaldenhoven]

@santiagohermo I tried to reproduce this markdown and playing with the method argument, but I am getting an error when trying to run sm_path.R - Can you point me to the code that you ran to produce the table here?

[santiagohermo]

Thanks for the review @SimonFreyaldenhoven! I went through your inline comments above. On your other comments.

The code looks good to me, so I have nothing beyond a couple of very minor comments, expect for one question: Did we already try a different method than Nelder and Mead?

We did not. The available methods can be seen under "Details" in this page. Do see any that you would like to try?

@santiagohermo I tried to reproduce this markdown and playing with the method argument, but I am getting an error when trying to run sm_path.R - Can you point me to the code that you ran to produce the table here?

The reason is that, to produce the markdown (especially the csv in the issue folder), I made a few small modifications to the package (so that EventStudyPlot returns Wald values and poly order and the package doesn't stop when the path is outside the region). Those changes can be seen in this commit. You should re-implement them and after that the issue folder should run.

If you think that some of these changes are useful we could make them more permanent under a verbose=TRUE argument. Let me know!

[nateschor]

Per call;

@santiagohermo will handle looping through the different smoothest path optimizers to see if that helps and will regenerate the markdown table for each of the different optimizers

FYI @SimonFreyaldenhoven

[SimonFreyaldenhoven]

@santiagohermo @nateschor hold off for now, I think I found the problem!

[SimonFreyaldenhoven]

@santiagohermo @jmshapir @nateschor updated convergence table here:

Looks like we have convergence everywhere now! 🥳

I'll add some more context this afternoon.

[SimonFreyaldenhoven]

@santiagohermo @jmshapir There was a small bug at the end of the optimization. After substituting in the constraints, we have the following:

Let's ignore the sign indeterminacy for now and assume we always have a plus in (22). We want to minimize the absolute value of v_2, so in practice we actually minimize f(v_b)=v^2_2. The solution to that optimization yields (v_b^ and f(v_b^)) as argmin and min respectively.

Before we then set v_2^ = sqrt(f(v_b^)). But note that this is not unique. There is another sign indeterminacy here since the square root can be both negative and positive. We always chose the positive one, so I guess we had a 50% chance of picking the correct sign. For example, in the case of a quadratic polynomial, by always using the positive coefficient on v_2 we forced the curvature to be positive. In the examples in the markdown that happened to be the wrong direction, which is why we never found a solution.

Anyways, the fix was simple. After we obtain the solution (v_b^ and f(v_b^ )) as argmin and min respectively, we then just compute v_2^ by plugging v_b^ into (22).

I implemented this here, branching out at this commit where @santiagohermo had made some tweaks to produce this markdown. I was only able to produce an html file using the plots.Rmd script, so I can't attach it or link to it in a user-friendly way, but the table above it taken from that new html version of the same markdown.

@jorpppp fyi - Could you check if xtevent does this step correctly? I also found this markdown really helpful in understanding the issue. Do you think we could produce something similar using xtevent?

[santiagohermo]

Before we then set v_2^ = sqrt(f(v_b^)). But note that this is not unique. There is another sign indeterminacy here since the square root can be both negative and positive. We always chose the positive one, so I guess we had a 50% chance of picking the correct sign. For example, in the case of a quadratic polynomial, by always using the positive coefficient on v_2 we forced the curvature to be positive. In the examples in the markdown that happened to be the wrong direction, which is why we never found a solution.

Wow @SimonFreyaldenhoven, that's a really cool realization. Thanks for the detailed exploration, and great that you caught this error! I will implement your solution in the main branch and rebuild the issue folder in markdown format.

[santiagohermo]

Alright, I implemented the changes suggested by @SimonFreyaldenhoven in https://github.com/JMSLab/eventstudyr/pull/23/commits/2e82bcdb2f35d8dad991a8c79d158b0ae71821bd. Since it's no longer needed, I also dropped the test that generated this thread in https://github.com/JMSLab/eventstudyr/pull/23/commits/e672e4d21f0becd4a72dec70bc109470263c21de.

The new markdown is here (added in https://github.com/JMSLab/eventstudyr/pull/23/commits/fdac5bd30b363fe5b00121d2dc24a226552cb5d6). The function returns a path inside the Wald region in all cases!

[SimonFreyaldenhoven]

@santiagohermo Everything looks great from my side too! thanks!

[jmshapir]

@SimonFreyaldenhoven thanks!

@santiagohermo can you wrap us up here?

[santiagohermo]

Thanks @jmshapir @SimonFreyaldenhoven! Wrapping up #23.

[santiagohermo]

Confirming that all tests pass.

check() returns a warning related to the vignette, fyi @nateschor and @rcalvo12. It doesn't look like an important issue so I'll proceed to wrap-up. We can revise in #17 if necessary.

[santiagohermo]

Branch issue_SF created in this comment deleted.

Summary comment is here.