X Tutup
The Wayback Machine - https://web.archive.org/web/20220218120552/https://github.com/scikit-learn/scikit-learn/pull/18898
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 allows TransformedTargetRegressor to take nD target #18898

Merged
merged 10 commits into from Apr 9, 2021
Merged

FIX allows TransformedTargetRegressor to take nD target #18898

merged 10 commits into from Apr 9, 2021

Conversation

@panangam
Copy link
Contributor

@panangam panangam commented Nov 23, 2020 •
edited

Reference Issues/PRs

Fixes #18866

What does this implement/fix? Explain your changes.

This removes check_array call from the fit method of TransformedTargetRegressor and relegate target validation to the transformer. The main motivation was the allow nD target to be used. But this also potentially allows transformers that deal with string or other data types to be used.

Any other comments?

This is my first scikit-learn PR so any feedback welcome!
@panangam panangam changed the title Allows TransformedTargetRegressor to take nD target [MRG]Allows TransformedTargetRegressor to take nD target Nov 24, 2020
@panangam panangam changed the title [MRG]Allows TransformedTargetRegressor to take nD target [MRG] Allows TransformedTargetRegressor to take nD target Nov 24, 2020
@glemaitre glemaitre self-requested a review Dec 18, 2020
glemaitre
glemaitre reviewed Dec 18, 2020
View changes
Copy link
Contributor

@glemaitre glemaitre left a comment

I think that we should still keep the check_array as a safe guard for the moment. We can style accomodate your use case by allowing nd array in check_array. We can later see if we should do less validation and delegate the y validation to the transformer itself.

Please add an entry to the change log at doc/whats_new/v*.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

sklearn/compose/_target.py Outdated Show resolved Hide resolved
sklearn/compose/tests/test_target.py Show resolved Hide resolved
@glemaitre glemaitre changed the title [MRG] Allows TransformedTargetRegressor to take nD target FIX allows TransformedTargetRegressor to take nD target with adequate transformer Dec 18, 2020
@cmarmo
Copy link
Member

@cmarmo cmarmo commented Jan 19, 2021

Hi @panangam would you be able to address the comments from the reviewer? Thanks! Feel free to ask if you need some clarifications.

Base automatically changed from master to main Jan 22, 2021
@panangam
Copy link
Contributor Author

@panangam panangam commented Feb 28, 2021

@glemaitre @cmarmo I know it's been forever but I've finally addressed the comment, bringing check_array back and did the book keeping stuff (commenting, changelogs).

@cmarmo
Copy link
Member

@cmarmo cmarmo commented Feb 28, 2021

Hi @panangam thanks for your updates. 0.24 has been released in December 2020, do you mind moving your changelog in doc/whats_new/v1.0.rst? Thanks!

@panangam
Copy link
Contributor Author

@panangam panangam commented Mar 1, 2021

Hi @panangam thanks for your updates. 0.24 has been released in December 2020, do you mind moving your changelog in doc/whats_new/v1.0.rst? Thanks!

Oh whoa, we're getting to 1.0? Will do!

@glemaitre glemaitre self-requested a review Apr 9, 2021
glemaitre
glemaitre approved these changes Apr 9, 2021
View changes
Copy link
Contributor

@glemaitre glemaitre left a comment

LGTM

thomasjpfan
thomasjpfan approved these changes Apr 9, 2021
View changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Thank you for the PR @panangam !

LGTM

@thomasjpfan thomasjpfan changed the title FIX allows TransformedTargetRegressor to take nD target with adequate transformer FIX allows TransformedTargetRegressor to take nD target Apr 9, 2021
@thomasjpfan thomasjpfan merged commit 3ff1267 into scikit-learn:main Apr 9, 2021
26 checks passed
thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this issue Apr 19, 2021
@panangam @glemaitre @thomasjpfan
FIX allows TransformedTargetRegressor to take nD target (scikit-learn…
83b8d2e 
…#18898)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@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

@glemaitre glemaitre

Assignees
No one assigned
Labels
module:compose Waiting for Reviewer
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Fitting a TransformedTargetRegressor to 3D target fails
4 participants
@panangam @cmarmo @thomasjpfan @glemaitre
X Tutup