-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add built-in column-specific transformers to TableVectorizer
#583
Add built-in column-specific transformers to TableVectorizer
#583
Conversation
column_specific_transformers
to TableVectorizer
TableVectorizer
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.
Very nice! A few minor comments.
One major one: we should use this in an example. Maybe by modifying an existing one
You have failing tests. Can you address them please |
Thanks for reminding me :) |
…v' into add_column_specific_transformers_tv
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.
Thanks @LilianBoulard, nice to see this implemented :)
A few comments for the example
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.
Hey Lilian, here is my first round of reviews! The example looks neat, let's add the plots from the results of the grid search :)
Co-authored-by: Vincent M <[email protected]> Co-authored-by: Jovan Stojanovic <[email protected]>
…com/LilianBoulard/skrub into add_column_specific_transformers_tv
The grid-search doesn't work as expected, there is something different between the |
…olumn_specific_transformers_tv
So the feature has been ready for a while, but this example is blocking. Since it doesn't work and needs some more work, I've put it into a temporary directory, so it's not accessible on the website, but so we don't lose the code. I'll open an issue in a few minutes to fix that. |
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.
I agree that we need to move on, until we find a good solution for the grid search.
Add one more small test before approving this :)
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 @LilianBoulard! After adding the missing test, I'm happy with this PR as-is. Let's investigate the grid-search on a different PR :)
…com/LilianBoulard/skrub into add_column_specific_transformers_tv
…olumn_specific_transformers_tv
Co-authored-by: Vincent M <[email protected]>
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.
Let's merge once the CI is green
Fixes part of #554
As discussed in #580, adds the
column_specific_transformers
parameter to theTableVectorizer
.This allows this kind of functionality:
from the
TableVectorizer
directly, with this syntax:When the assignements need to be named (e.g. for a grid-search), the user can specify a name (same syntax as the
ColumnTransformer
):