-
Notifications
You must be signed in to change notification settings - Fork 192
chore: Refactor SingleColumnTransformer so it's in its own file #1820
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
Conversation
…ted imports and functions
|
Hi @techy4shri, thanks a lot for this PR! It's already in a good shape. The tests are failing because doctests are checking that the examples in the documentations are passing. Doctests are failing in some places because they are expecting the RejectColumn exception to still be in skrub._apply_to_cols, while now it has been moved to the new file. Those doctests should therefore be updated with the new path to not fail: skrub._apply_to_cols.RejectColumn: should become skrub_single_column_transformer.RejectColumn: Also, could you move the tests on the single_column_transformer to their own separate test file? |
|
Understood @rcap107 ! Will do! Thanks for the confirmation. |
|
@rcap107 the PR is ready for review, kindly let me know if any changes are required. |
rcap107
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks a lot @techy4shri!
Dismissing to give some more time to other maintainers
|
okk thanks! I shall be waiting for the final judgement 🫡 @jeromedockes do let me know your thoughts too 👀 |
jeromedockes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for all this work @techy4shri ! 🚀
|
Pre-commit check will fail now because of line length. |
|
Hi @techy4shri, thanks for the PR, and I'm sorry for the spotty support these past weeks. The PR should be ready to merge once the precommit checks pass, which should be easy to fix with |
|
@rcap107 @jeromedockes all checks are passing now! |
|
I saw, thanks a lot @techy4shri. The PR looks good to me, I'm waiting for @jeromedockes to check if his comments have been addressed before merging. |
|
It's been a couple of days and this PR is currently blocking a few others, so I'll merge it. If needed, we can add minor changes in a follow-up. Thanks a lot @techy4shri for the work and @jeromedockes for the review. |
Closes #1729
Now the new file has separate transformer classes, the tests all but pass. I refrained from making a new test file since
test_on_each_column.pyhas bothSingleColumnTransformerandRejectColumncovered in it.Let me know if any changes are required.
Note: The current implementation is such that both SingleColumnTransformer and RejectColumn are added as an internal libs rather than being part of the public API. I read the issue and the long discussion about making them both public in this issue discussion but it seems to me that the internal team would handle it later, after the issue I am assigned to will be closed. Just putting it here :)