Skip to content

FIX Only raise feature name warning with mixed types and strings #22410

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

Merged
merged 4 commits into from
Feb 8, 2022

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Closes #22399

What does this implement/fix? Explain your changes.

This PR adjusts the check to only raise a warning for feature names when there is a mixed dtype and a string is in it.

Any other comments?

I think this is more consistent with the @ogrisel's comment here: #18010 (comment)

CC @glemaitre

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

Comment on lines 49 to 50
no longer warns when passed into an Estimator. These non-string columns
will be supported in 1.3. Note that `feature_names_in_` will continue to
Copy link
Member

Choose a reason for hiding this comment

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

how will they be supported?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated PR to clarify what supported means:

Estimators will continue to ignore the column names in DataFrames with non-string columns. For
feature_names_in_ to be defined, columns must be all strings

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM. I still agree with my previous comment, which is somewhat re-assuring as it's not always the case ;)

Thanks for fixing this.

@ogrisel ogrisel merged commit c0c504d into scikit-learn:main Feb 8, 2022
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Feb 9, 2022
ilivans pushed a commit to ilivans/scikit-learn that referenced this pull request Feb 10, 2022
thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this pull request Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants