-
-
Notifications
You must be signed in to change notification settings - Fork 26k
[MRG] DEP Deprecate None in FeatureUnion #15053
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
[MRG] DEP Deprecate None in FeatureUnion #15053
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs a whats_new entry
sklearn/tests/test_pipeline.py
Outdated
@@ -907,7 +908,9 @@ def test_set_feature_union_steps(): | |||
assert ['mock__x5'] == ft.get_feature_names() | |||
|
|||
|
|||
# TODO: Remove in 0.24 when 'drop' is removed for FeatureUnion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop or None?
sklearn/tests/test_pipeline.py
Outdated
@pytest.mark.parametrize('drop', ['drop', None]) | ||
@ignore_warnings(category=DeprecationWarning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we fix the test instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @thomasjpfan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits but LGTM
If we're going to merge this one + the Voting one, I think we should deprecate that for pipelines too
sklearn/tests/test_pipeline.py
Outdated
|
||
|
||
# TODO: Remove in 0.24 when None is removed | ||
def test_feature_union_warns_with_drop(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_feature_union_warns_with_drop(): | |
def test_feature_union_warns_with_none(): |
sklearn/tests/test_pipeline.py
Outdated
@@ -907,6 +907,7 @@ def test_set_feature_union_steps(): | |||
assert ['mock__x5'] == ft.get_feature_names() | |||
|
|||
|
|||
# TODO: Remove in 0.24 when None is removed for FeatureUnion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# TODO: Remove in 0.24 when None is removed for FeatureUnion | |
# TODO: Remove parametrization in 0.24 when None is removed for FeatureUnion |
(we don't want to remove the test!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix merge conflicts. LGTM, otherwise.
linter error. Otherwise LGTM |
Reference Issues/PRs
Related to #14813
What does this implement/fix? Explain your changes.
Deprecates
None
inFeatureUnion
.Any other comments?
Two options to do the same thing is undesirable.