Skip to content

Move _feols_input_checks functions from feols.py to dev_utils.py #873

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

csanthoshkumarraju
Copy link

Hi,

Issue Addressed:

This PR addresses the issue in #869, where all functions defined at the end of feols.py, starting with _feols_input_checks, should be moved into the dev_utils.py script.

Summary of Changes:

Moved the _feols_input_checks function from the end of feols.py to dev_utils.py to improve modularity and code organization.
Updated the necessary imports in feols.py to import the _feols_input_checks function from dev_utils.py.

Detailed Explanation:

The _feols_input_checks function was previously located at the end of feols.py. I have commented it out in feols.py and moved it to dev_utils.py for better structure and reusability.
After moving the function, I imported it into feols.py to ensure no disruptions in functionality.

Screenshot 2025-04-06 at 10 25 29 AM Screenshot 2025-04-06 at 10 26 50 AM Screenshot 2025-04-06 at 10 30 56 AM Screenshot 2025-04-06 at 10 28 19 AM

Testing:

All existing tests were executed and passed successfully after making the changes.
The pixi run lint command showed no linting issues, ensuring code consistency.

Screenshot 2025-04-06 at 10 43 31 AM

This is my first open-source contribution. Could you please check the pull request and merge it if everything is in order?

Thank you!

Copy link

codecov bot commented Apr 6, 2025

Codecov Report

Attention: Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pyfixest/utils/dev_utils.py 7.69% 12 Missing ⚠️
Flag Coverage Δ
core-tests 79.84% <14.28%> (ø)
tests-extended ?
tests-vs-r 47.83% <14.28%> (ø)

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

Files with missing lines Coverage Δ
pyfixest/estimation/feols_.py 84.59% <ø> (-5.78%) ⬇️
pyfixest/estimation/fepois_.py 90.04% <100.00%> (ø)
pyfixest/estimation/vcov_utils.py 57.89% <ø> (ø)
pyfixest/utils/dev_utils.py 72.41% <7.69%> (-11.37%) ⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@csanthoshkumarraju
Copy link
Author

pre-commit.ci autofix

@s3alfisc
Copy link
Member

s3alfisc commented Apr 6, 2025

Hi @csanthoshkumarraju, thanks so much for this! Looks good at a first glance. Could you delete all the coverage files?

@csanthoshkumarraju
Copy link
Author

Hi @s3alfisc ,

Thank you so much for the feedback! I really appreciate it and it motivates me to keep improving.

As requested, I have deleted the coverage files. Could you kindly review the changes and merge the pull request if everything looks good? If you need any further assistance or clarification, feel free to let me know.

Also, I’ve attached the screenshot below for your reference.
Screenshot 2025-04-06 at 2 42 10 PM

Thanks again!

Best regards,
Santhosh

@s3alfisc
Copy link
Member

s3alfisc commented Apr 6, 2025

Hi, I meant all these files - can you please delete them / delete them from the commit history? Then I think this will be ready to be merged. Thanks!
image

@s3alfisc
Copy link
Member

s3alfisc commented Apr 6, 2025

pre-commit.ci autofix

@csanthoshkumarraju
Copy link
Author

Hi @s3alfisc

I apologize for the confusion earlier. I have deleted all the files as requested. Could you please check the screenshots?
Screenshot 2025-04-06 at 4 53 23 PM
Screenshot 2025-04-06 at 4 48 48 PM

If everything looks good, could you please merge my pull request?

change gitignore in coverage
@s3alfisc
Copy link
Member

s3alfisc commented Apr 6, 2025

If everything looks good, could you please merge my pull request?

Sorry, I don't really follow, why the urgency to merge this?

@csanthoshkumarraju
Copy link
Author

Hi @s3alfisc

I’m sorry again for any confusion. There is no urgency in merging the pull request. I’m just kindly requesting if you could please review and merge it when you have the time.

Thank you!

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