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.

Sign up for GitHub

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

Jump to bottom

Improve SuperVectorizer #201

Merged
merged 27 commits into from
Oct 8, 2021
Merged

Improve SuperVectorizer #201

merged 27 commits into from
Oct 8, 2021

Conversation

LilianBoulard
Copy link
Member

@LilianBoulard LilianBoulard commented Jul 23, 2021 •
edited

Implements various improvements in the SuperVectorizer, such as :

@LilianBoulard LilianBoulard marked this pull request as draft July 23, 2021 14:06
@LilianBoulard LilianBoulard marked this pull request as ready for review July 29, 2021 11:36
@LilianBoulard LilianBoulard changed the title Don't handle missing values in the SuperVectorizer Improve SuperVectorizer Jul 30, 2021
@LilianBoulard
Copy link
Member Author

Updated original comment to describe the changes of d5f6193
I still have a bit of work to do, especially improving the tests and documenting the new properties.

@LilianBoulard LilianBoulard added the enhancement New feature or request label Aug 2, 2021
GaelVaroquaux
GaelVaroquaux reviewed Sep 1, 2021
View reviewed changes
Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review. I'll finish this ASAP

dirty_cat/super_vectorizer.py Outdated Show resolved Hide resolved
dirty_cat/super_vectorizer.py Show resolved Hide resolved
dirty_cat/super_vectorizer.py Outdated Show resolved Hide resolved
dirty_cat/super_vectorizer.py Show resolved Hide resolved
dirty_cat/super_vectorizer.py Outdated Show resolved Hide resolved
dirty_cat/super_vectorizer.py Outdated Show resolved Hide resolved
dirty_cat/super_vectorizer.py Outdated Show resolved Hide resolved
dirty_cat/super_vectorizer.py Outdated Show resolved Hide resolved
GaelVaroquaux
GaelVaroquaux reviewed Sep 2, 2021
View reviewed changes
Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I finished going through all the code, and I put in all my comments. This PR will be good to go.

@GaelVaroquaux
Copy link
Member

GaelVaroquaux commented Sep 2, 2021 via email

@alexis-cvetkov
Copy link
Contributor

I finished reviewing the PR:

  • we need to update CHANGES.rst
  • maybe add a test to ensure that fit_transform(X) is equivalent to fit(X).transform(X)
  • is there a reason to define a separate encoder for string column and categorical columns in the parameters of the SuperVectorizer ?

GaelVaroquaux
GaelVaroquaux reviewed Oct 7, 2021
View reviewed changes
Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@GaelVaroquaux
Copy link
Member

The questions of Alexis are very relevant. Once they are addressed, I am +1 for merge.

@LilianBoulard
Copy link
Member Author

LilianBoulard commented Oct 8, 2021 •
edited

Alexis' questions are addressed by the last commits, thanks for the feedback !

@GaelVaroquaux GaelVaroquaux added this to the 0.2.0 release milestone Oct 8, 2021
@alexis-cvetkov alexis-cvetkov merged commit 932fd23 into skrub-data:master Oct 8, 2021
@alexis-cvetkov
Copy link
Contributor

Merging !

@LilianBoulard LilianBoulard deleted the fix_sv_na_values branch October 12, 2021 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GaelVaroquaux GaelVaroquaux GaelVaroquaux left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
0.2.0 release
Development

Successfully merging this pull request may close these issues.

Missing values replaced by the SuperVectorizer
3 participants
@LilianBoulard @GaelVaroquaux @alexis-cvetkov