Skip to content
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.

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

Adding westfall-young p-value adjustment for multiple tests #725

Merged
merged 13 commits into from
Dec 4, 2024

Conversation

marcandre259
Copy link
Contributor

From closed pr #698

Takes in most of @s3alfisc review comments.
Tests are still a weak point.
Possible issue with Randomization Inference -> #717

Copy link

codecov bot commented Nov 23, 2024

Codecov Report

Attention: Patch coverage is 65.57377% with 21 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pyfixest/estimation/multcomp.py 65.57% 21 Missing ⚠️
Flag Coverage Δ
core-tests 78.01% <65.57%> (-0.33%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pyfixest/__init__.py 81.81% <ø> (ø)
pyfixest/estimation/__init__.py 100.00% <ø> (ø)
pyfixest/estimation/multcomp.py 66.08% <65.57%> (-12.40%) ⬇️

@marcandre259
Copy link
Contributor Author

pre-commit.ci autofix

@s3alfisc
Copy link
Member

Thanks @marcandre259 , I'll review this evening!

@s3alfisc
Copy link
Member

Hi, this looks very good, thank you @marcandre259! I left a tiny comment that I fixed myself =) I might also add some unit tests tomorrow and think again about the issue of RI vs the bootstrap you raised in the other PR. What is your current thinking on this at the moment? Do you think it is a bug, or are we being fooled by statistics?

@marcandre259
Copy link
Contributor Author

Hi, this looks very good, thank you @marcandre259! I left a tiny comment that I fixed myself =) I might also add some unit tests tomorrow and think again about the issue of RI vs the bootstrap you raised in the other PR. What is your current thinking on this at the moment? Do you think it is a bug, or are we being fooled by statistics?

Thanks for looking into it @s3alfisc :) I can also add tests if needed.

Regarding the (maybe) issue, for me what is weird is getting this super tight null distribution. I figure maybe I'm missing something. In any case, there are probably tons of randomization sampling programs out there to do comparisons, so if it's still hanging, I'll look into it this weekend.

@s3alfisc
Copy link
Member

Thanks for tackling the tests!

But the null distribution seems to fit the analytical SEs? The tests against ritest are here, though I only check p-values as far as I recall, not null distribution widths.

@s3alfisc
Copy link
Member

Actually there are tests for the SEs of the ritest t-stats and the lower bound of its ci implemented as well:

def test_vs_r(data, fml, resampvar, cluster, ritest_results):
Though if you have access to a Stata license, would of course be great to check against Stata ritest as well!

@s3alfisc
Copy link
Member

s3alfisc commented Dec 3, 2024

Now that I've found the ritest bug and fixed it in #730, I will take another look at this & implement some tests tomorrow evening. 👍

@s3alfisc
Copy link
Member

s3alfisc commented Dec 4, 2024

pre-commit.ci autofix

pyfixest/estimation/multcomp.py Outdated Show resolved Hide resolved
pyfixest/estimation/multcomp.py Show resolved Hide resolved
@s3alfisc s3alfisc merged commit 611e5cc into py-econometrics:master Dec 4, 2024
7 of 8 checks passed
@s3alfisc
Copy link
Member

s3alfisc commented Dec 4, 2024

And merged. Thanks so much @marcandre259!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants