Skip to content

Add stop-words to StringEncoder#1415

Merged
Vincent-Maladiere merged 6 commits into
skrub-data:mainfrom
Vincent-Maladiere:add_stopwords_string_encoder
Jun 4, 2025
Merged

Add stop-words to StringEncoder#1415
Vincent-Maladiere merged 6 commits into
skrub-data:mainfrom
Vincent-Maladiere:add_stopwords_string_encoder

Conversation

@Vincent-Maladiere

Copy link
Copy Markdown
Member

Fixes #1402 and add a small footnote in the StringEncoder.

@ArturoAmorQ ArturoAmorQ left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not a maintainer here, but if I may have a comment, how about we also mention in the dosctring

If None, no stop words will be used.

Comment thread skrub/_string_encoder.py Outdated
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
@Vincent-Maladiere

Copy link
Copy Markdown
Member Author

You're right! I scratched that part because I would have to mention the max_df parameter which we don't expose; but maybe I don't have to

Comment thread CHANGES.rst Outdated

Changes
-------
- :class:`StringEncoder` now exposes the `stop_words` argument, which is passed to the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the warning should remain at the top of the Changes section

Comment thread skrub/tests/test_string_encoder.py

@rcap107 rcap107 left a comment

Copy link
Copy Markdown
Member

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 @Vincent-Maladiere ! This should cover a lot of use cases.

@Vincent-Maladiere

Copy link
Copy Markdown
Member Author

Merging, as @GaelVaroquaux agreed to this PR during the skrub meeting

@Vincent-Maladiere
Vincent-Maladiere merged commit 9969b98 into skrub-data:main Jun 4, 2025
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.

More parameters to control the StringEncoder underlying estimators?

3 participants