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.

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

FIX Encoder should accept categories having dtype='S' #19727

Merged
merged 8 commits into from Apr 21, 2021
Merged

FIX Encoder should accept categories having dtype='S' #19727

merged 8 commits into from Apr 21, 2021

Conversation

andrewdelong
Copy link
Contributor

@andrewdelong andrewdelong commented Mar 19, 2021 •
edited

Reference Issues/PRs

Fixes #19677, which is a regression in how OneHotEncoder and OrdinalEncoder handle categories having dtype='S'.

What does this implement/fix? Explain your changes.

  • Changed the dtype.kind checks from 'OU' to 'OUS' and updated the test.
  • To facilitate the test, also added a dtype arg to _convert_container. Examples:
>>> _convert_container([['a'], ['b']], "list", dtype='S')
[[b'a'], [b'b']]
>>> _convert_container([[1], [2]], "list", dtype=np.float32)
[[1.], [2.]]

@andrewdelong
fix 19677: specify categories with dtype='S'
f9a8283 
- add dtype='S' cases to `test_encoders_string_categories`
- add dtype arg to `_convert_container` to force dtype when applicable
- `_convert_container` to list now converts container `[[1], [2]]` to requested dtype (e.g., `[[1.], [2.]]`)
@andrewdelong
flake8 compat
776c1df
@andrewdelong andrewdelong changed the title Fix encoder categories having dtype='S' [MRG] Fix encoder categories having dtype='S' Mar 19, 2021
thomasjpfan
thomasjpfan reviewed Mar 22, 2021
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @andrewdelong !

sklearn/preprocessing/tests/test_encoders.py Outdated Show resolved Hide resolved
sklearn/preprocessing/tests/test_encoders.py Outdated Show resolved Hide resolved
sklearn/utils/_testing.py Outdated Show resolved Hide resolved
@cmarmo cmarmo added this to the 0.24.2 milestone Mar 23, 2021
@glemaitre glemaitre self-assigned this Apr 21, 2021
@glemaitre glemaitre changed the title [MRG] Fix encoder categories having dtype='S' FIX encoder should accept categories having dtype='S' Apr 21, 2021
@glemaitre glemaitre added To backport PR merged in master that need a backport to a release branch defined based on the milestone. and removed Waiting for Reviewer labels Apr 21, 2021
@glemaitre glemaitre removed their assignment Apr 21, 2021
glemaitre
glemaitre approved these changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM. Oh, actually we miss a second approval. @thomasjpfan do you want to have a new look at it.

@thomasjpfan thomasjpfan changed the title FIX encoder should accept categories having dtype='S' FIX Encoder should accept categories having dtype='S' Apr 21, 2021
thomasjpfan
thomasjpfan approved these changes Apr 21, 2021
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM!

Thank you for working on this @andrewdelong !

@thomasjpfan thomasjpfan merged commit a67b284 into scikit-learn:main Apr 21, 2021
29 checks passed
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Apr 22, 2021
@andrewdelong @glemaitre
FIX Encoder should accept categories having dtype='S' (scikit-learn#1…
41a8d1f 
…9727)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
glemaitre added a commit that referenced this pull request Apr 28, 2021
@andrewdelong @glemaitre
FIX Encoder should accept categories having dtype='S' (#19727)
bd1afb6 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
module:preprocessing module:utils To backport PR merged in master that need a backport to a release branch defined based on the milestone.
Projects
None yet
Milestone
0.24.2
Development

Successfully merging this pull request may close these issues.

OneHot/OrdinalEncoder categories broken for dtype='S'
4 participants
@andrewdelong @thomasjpfan @glemaitre @cmarmo