Skip to content

FIX Adds check_array to inverse_transform of StandardScaler #19356

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
Merged
merged 11 commits into from
Mar 23, 2021
Merged

FIX Adds check_array to inverse_transform of StandardScaler #19356

merged 11 commits into from
Mar 23, 2021

Conversation

makoeppel
Copy link
Contributor

@makoeppel makoeppel commented Feb 5, 2021 •
edited by thomasjpfan
Loading

Reference Issues/PRs

Fixes #19354

What does this implement/fix? Explain your changes.

This PR checks the input array of the inverse_transform function of the StandardScaler similar like its done in the MinMaxScaler.

thomasjpfan
thomasjpfan reviewed Feb 5, 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.

Please add a non-regression test that would fail at main but pass in this PR.

@cmarmo
Copy link
Contributor

cmarmo commented Feb 11, 2021

Hi @makoeppel, adding a check_array makes the test test_transform_target_regressor_2d_transformer[X0-y0] fail. Do you mind having a look? Thanks!

@makoeppel
Copy link
Contributor Author

makoeppel commented Feb 11, 2021 •
edited
Loading

I added ensure_2d=False into the check_array function which fixes the test_transform_target_regressor_2d_transformer[X0-y0] test.

lorentzenchr
lorentzenchr approved these changes Feb 15, 2021
View reviewed changes
Copy link
Member

@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasjpfan thomasjpfan mentioned this pull request Feb 21, 2021
Closed
thomasjpfan
thomasjpfan reviewed Feb 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.

I think ensure_2d should be True and the current behavior is a bug. test_transform_target_regressor_2d_transformer is taking advantage of this bug. Unfortunately, setting ensure_2d=True would be not be backward compatible. I opened #19518 to discuss this issue.

For now, we can merge this as it resolves the dtype issue and is backward compatible.

thomasjpfan
thomasjpfan approved these changes Feb 22, 2021
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please add an entry to the change log at doc/whats_new/v1.0.rst with tag |Fix|. Like the other entries there, please reference this pull request with :pr: and credit yourself with :user:.

@thomasjpfan thomasjpfan changed the title add check_array to inverse_transform of StandardScaler FIX Adds check_array to inverse_transform of StandardScaler Feb 22, 2021
@thomasjpfan thomasjpfan force-pushed the check_array_StandardScaler branch from 330cb1e to 17dd5be Compare February 23, 2021 18:23
@thomasjpfan thomasjpfan merged commit df3f1bd into scikit-learn:main Mar 23, 2021
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@lorentzenchr lorentzenchr lorentzenchr approved these changes

Assignees
No one assigned
Labels
module:preprocessing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Input format for StandardScaler
4 participants
@makoeppel @cmarmo @thomasjpfan @lorentzenchr