Add "ri" resampling method to Romano-Wolf procedure,

[Jayhyung]

Hi, Alex.

I made some changes to the function rwolf to allow resampling method when computing p-value using Romano-Wolf procedure.

There are a couple of concerns that I have regarding this change. I would appreciate if you could review this, and give me the feedback(if these are okay to ignore, please link the pull request to the repo!)

1. The comment in the original code(multicomp.py in estimation folder) was somewhat tailored to the bootstrapping method. For example, the comment section in the function _get_rwolf_pval has this comment "Compute Romano-Wolf adjusted p-values based on bootstrapped t-statistics.". Please, confirm that I can use this function to compute the RW adjusted p-value with "ri" method as I did in the line 177. According to my understanding of this paper, the RW procedure should be exactly the same for both resampling methods except for the part that we are using different sampling method for computing t-statistics from resampling.

2. Do you think I should add a testing file to check on whether the two different resampling method produces similar results? I'm not sure how to do this, so I would appreciate it if you could give me any example code for testing. I think comparing the adjusted p-values from both methods would be enough.

Thanks!

[s3alfisc]

Hi! This looks pretty good! 🎉 thank you =)

Indeed, the rwolf stepwise function should work for both bootstrap and randomization inference p values. =)

Regarding tests - one could argue that the feature does not need to be tested as both the rwolf function and the rotest function are tested independently.

But it's likely best to have a test anyways (I've learned that I should test everything, if not, it will bite us) 😀 the best place for tests is in test_multcomp.py. You can start with the tolerance thresholds and iterations there (and please tell me if you feel we should be stricter with the test 😅)

For testing, I think you're strategy is the right one - under a dgp where the RI and bootstrap assumptions hold (ie treatment being conditionally randomly assigned), you should expect rwolf to produce the same pvalue, no matter if run via a bootstrap or randomization inference.

[Jayhyung]

Thanks for the comment! Yes, I think that we should test as many as possible. I also think that I did really many dumb things by not testing my codes in the past ;). I will prepare the test code. Stay tuned! :)

[Jayhyung]

I made a minor change to multcomp.py and added a test code to test_multcomp.py. The newly added test passed on my local machine. Please let me know if I need to do any extra works on this PR. Thanks!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Files	Coverage Δ
pyfixest/estimation/multcomp.py	98.68% <100.00%> (ø)
tests/test_errors.py	98.67% <100.00%> (+1.59%)	⬆️
tests/test_multcomp.py	91.66% <100.00%> (ø)

... and 27 files with indirect coverage changes

[s3alfisc]

Thanks @Jayhyung 🎉I added a few more small comments. Very nicely, the CI tests ran succesfully! So I think we will be able to merge this after the next iteration =)

[Jayhyung]

Thanks for the comment. I modified the code, and added the test according to you suggestion. Please, let me know if anything extra should be done. Thanks!

[s3alfisc]

Looks great! Will merge this after the ci runs. Congrats to your first PR to pyfixest 🎉 very cool!

@all-contributors please add @Jayhyung for code

[allcontributors]

@s3alfisc

I've put up a pull request to add @Jayhyung! 🎉

[s3alfisc]

And merged! 🥳