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

Run more tests #603

Merged
merged 10 commits into from
Sep 7, 2024
Merged

Run more tests #603

merged 10 commits into from
Sep 7, 2024

Conversation

s3alfisc
Copy link
Member

@s3alfisc s3alfisc commented Sep 1, 2024

  • Bring back tests I un-commented a while back.
  • Fix bug in first_stage() method with special formulaic syntax.
  • Fix bug with resid() method, which sometimes would return weighted residuals. Now it always returns the unweighted residuals.
  • Add tests for predict(), resid() methods and iwls_weights attributes.

@Jayhyung
Copy link
Member

Jayhyung commented Sep 1, 2024

Ah, I didn't notice that the IV part in test_vs_fixest.py was commented. I have worked on the seperate testing script(test_iv.py), so the diagnostic part of IV should work fine. I also see that you reworked on the first stage part at #601, which I think using FixestFormula is more reasonable to accommodate the complex formula syntax. If this indeed affects the other parts in the iv class, please let me know! I'm happy to rework on this.

@s3alfisc
Copy link
Member Author

s3alfisc commented Sep 2, 2024

Ah, I didn't notice that the IV part in test_vs_fixest.py was commented.

Yeah, me neither; a seemingly innocuuous "skip" statement and I asked pytest to skip all IV tests 🤦‍♂️Yeah, generally I agree that all is fine - coefs, SEs etc are all correct. I think something broke with C() operators - maybe best to change this in a separate PR from this one here and the testing PR? I might ask you to review it at some point this week if that's ok =)

@s3alfisc
Copy link
Member Author

s3alfisc commented Sep 2, 2024

Hi @Jayhyung , I pushed the changes to the IV module in d7a792c, could you take a look in case you find some time (only at the specific commit)?

@Jayhyung
Copy link
Member

Jayhyung commented Sep 3, 2024

Hi @s3alfisc ! I will check on this issue as soon as possible, possibly this weekend.

@s3alfisc
Copy link
Member Author

s3alfisc commented Sep 4, 2024

Very cool! I think reviewing the IV parts should not take you more than 15 minutes =)

@s3alfisc
Copy link
Member Author

s3alfisc commented Sep 7, 2024

Hi @Jayhyung, I will likely merge this in a few hours as I need this in another PR. If you find the time, would nevertheless be great if you'd review the IV part (we can always revert / update in case something is off / you think smth should be improved).

@Jayhyung
Copy link
Member

Jayhyung commented Sep 7, 2024

Thanks for the reminder, @s3alfisc ! Please, proceed without my review at this moment. Sorry, I will take a look at some point today, as I was swamped by my stuffs ;). If i find something weird or does not not work, I will make a new issue or PR.

@s3alfisc
Copy link
Member Author

s3alfisc commented Sep 7, 2024

Sounds good and no worries at all!

@s3alfisc s3alfisc merged commit faf992c into master Sep 7, 2024
8 checks passed
@s3alfisc s3alfisc deleted the more-tests branch September 7, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants