-
Notifications
You must be signed in to change notification settings - Fork 90
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
SimilarityEncoder cleanup #596
SimilarityEncoder cleanup #596
Conversation
9882a1d
to
54bcf12
Compare
Thanks! Do you mind using this PR for some more cleanup of the |
I would modify the "notes" section of the SimilarityEncoder as such "The functionality of :class: |
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.
Look's great to me. I've left a few comments, and in addition I agree that if you can tackle @LilianBoulard 's point its great.
Thanks!!
Co-authored-by: Gael Varoquaux <gael.varoquaux@normalesup.org>
I don't see any TODO @LilianBoulard. |
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.
Two tiny suggestions. +1 to merge on my side once they have been addressed.
@LilianBoulard : I'll let you do a final review and press the merge button if everything is fine on your side.
Co-authored-by: Gael Varoquaux <gael.varoquaux@normalesup.org>
Yes but we have a test which check that an estimator summary fits in one line.
Ah, darn such crazy rigidity.
"Encode string categories to a similarity matrix, to capture fuzziness across a few categories."
This is good
|
…rin/skrub into remove_old_features_sim_enc
fix #573