10000 FIX Remove validation for FeatureUnion from __init__ by iofall · Pull Request #21954 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

FIX Remove validation for FeatureUnion from __init__ #21954

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 6 commits into from
Jan 31, 2022
Merged

FIX Remove validation for FeatureUnion from __init__ #21954

merged 6 commits into from
Jan 31, 2022

Conversation

iofall
Copy link
Contributor
@iofall iofall commented Dec 11, 2021

Addresses #21406

What does this implement/fix? Explain your changes.

  1. Removed self._validate_transformers() from __init__ .
  2. self._validate_transformers() is already called in _parallel_func() which is used by both fit() and fit_transform()
  3. Thus validation is now not done in __init__

cc: @arisayosh

@iofall @arisayosh
Remove validation for FeatureUnion from __init__
4d6d17d 
Co-authored-by: arisayosh <15692997+arisayosh@users.noreply.github.com>
Co-authored-by: iofall <50991099+iofall@users.noreply.github.com>
iofall
iofall commented Dec 11, 2021
View reviewed changes
sklearn/tests/test_pipeline.py
Comment on lines +521 to +523
fs = FeatureUnion([("transform", Transf()), ("no_transform", NoTrans())])
with pytest.raises(TypeError, match=msg):
FeatureUnion([("transform", Transf()), ("no_transform", NoTrans())])
fs.fit(X)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We noticed while working on this PR that the transform method is not checked which is the intended check for this test. We tried using fit_transform() but it also has a skip case defined which allows it to work without a transform method.

Wondering if this should be fixed in a separate issue?

Copy link
Member

Choose a reason for hiding this comment

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

The test is checking that during fit or fit_*, the __init__ parameters are valid. NoTrans does not define a transform method so FeatureUnion should raise an error.

By convention, fs.transform does not validate __init__ parameters.

skip case defined which allows it to work without a transform method.

May you clarify what you mean by "skip case"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for clearing this 👍. On reading your comments and revisiting this, there seems to be no problem. There is no skip case, the _validate_transformers() method does go through all the validation as required by the test.

thomasjpfan
thomasjpfan reviewed Dec 15, 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.

Thank you for the PR @iofall !

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

sklearn/tests/test_pipeline.py
Comment on lines +521 to +523
fs = FeatureUnion([("transform", Transf()), ("no_transform", NoTrans())])
with pytest.raises(TypeError, match=msg):
FeatureUnion([("transform", Transf()), ("no_transform", NoTrans())])
fs.fit(X)
Copy link
Member

Choose a reason for hiding this comment

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

The test is checking that during fit or fit_*, the __init__ parameters are valid. NoTrans does not define a transform method so FeatureUnion should raise an error.

By convention, fs.transform does not validate __init__ parameters.

skip case defined which allows it to work without a transform method.

May you clarify what you mean by "skip case"?

thomasjpfan
thomasjpfan approved these changes Dec 17, 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 update @iofall !

LGTM

glemaitre
glemaitre approved these changes Jan 28, 2022
View reviewed changes
Copy link
Member
@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.

Otherwise LGTM

doc/whats_new/v1.1.rst Outdated
@@ -305,6 +305,10 @@ Changelog
`__init__` but in `.fit()`.
:pr:`21888` by :user:`iofall <iofall>` and :user: `Arisa Y. <arisayosh>`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:pr:`21888` by :user:`iofall <iofall>` and :user: `Arisa Y. <arisayosh>`.
:pr:`21888` by :user:`iofall <iofall>` and :user:`Arisa Y. <arisayosh>`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback I have made these changes!

doc/whats_new/v1.1.rst Outdated
@@ -305,6 +305,10 @@ Changelog
`__init__` but in `.fit()`.
:pr:`21888` by :user:`iofall <iofall>` and :user: `Arisa Y. <arisayosh>`.

- |Fix| :class: `pipeline.FeatureUnion` does not validate hyper-parameters in
`__init__`. Validation is now handled in `.fit()` and `.fit_transform()`.
:pr:`21954` by :user:`iofall <iofall>` and :user: `Arisa Y. <arisayosh>`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:pr:`21954` by :user:`iofall <iofall>` and :user: `Arisa Y. <arisayosh>`.
:pr:`21954` by :user:`iofall <iofall>` and :user:`Arisa Y. <arisayosh>`.

iofall and others added 2 commits January 28, 2022 22:49
@iofall @arisayosh
Update whats_new
c45843b 
Co-authored-by: iofall <50991099+iofall@users.noreply.github.com>
Co-authored-by: arisayosh <15692997+arisayosh@users.noreply.github.com>
@iofall
Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…
8d99d82 
…nto Issue-21406-FeatureUnion
glemaitre
glemaitre reviewed Jan 31, 2022
View reviewed changes
@glemaitre glemaitre changed the title ENH Remove validation for FeatureUnion from __init__ FIX Remove validation for FeatureUnion from __init__ Jan 31, 2022
@glemaitre glemaitre merged commit faff01b into scikit-learn:main Jan 31, 2022
@glemaitre
Copy link
Member

Thanks @iofall

iofall
iofall commented Jan 31, 2022
View reviewed changes
doc/whats_new/v1.1.rst
@@ -508,8 +508,8 @@ Changelog

- |Fix| :class: `pipeline.FeatureUnion` does not validate hyper-parameters in
`__init__`. Validation is now handled in `.fit()` and `.fit_transform()`.
:pr:`21954` by :user:`iofall <iofall>` and :user:`Arisa Y. <arisayosh>`.
:pr:`21888` by :user:`iofall <iofall>` and :user:`Arisa Y. <arisayosh>`.
:pr:`21954` and :pr:`21888` by :user:`iofall <iofall>` and
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@glemaitre, I realize after seeing this change that I had accidentally left the line for :pr:21888. Sorry about that. This line should only be:

:pr:`21954` by :user:`iofall <iofall>` and :user:`Arisa Y. <arisayosh>`.

How should we fix this? Should I add the commit on this PR or create a new PR?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see no worries. I will make a small doc fix in main.

Copy link
Member

Choose a reason for hiding this comment

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

FYI, it will be fixed in #22347

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

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:pipeline
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iofall @glemaitre @thomasjpfan
0