[MRG+1] Adds drop in FeatureUnion #11640
Conversation
|
This is not backwards compatible. Otherwise it's okay. |
|
Would it be better to support 'drop' and |
|
I think that might be appropriate in the interim, or we can deprecate None
support with a warning for now.
|
|
I updated this PR to deprecate None. |
sklearn/pipeline.py
Outdated
| if show_warning: | ||
| warnings.warn( | ||
| "Transformers set to None is now set with 'drop' " | ||
| "in version 0.20 and will be removed in 0.22.", |
There was a problem hiding this comment.
I don't think this message is clear. Please reword it in a way that is helpful to someone who formerly used None
sklearn/pipeline.py
Outdated
| show_warning = False | ||
| for idx, (name, trans) in enumerate(self.transformer_list): | ||
| if trans is None: | ||
| self.transformer_list[idx] = (name, 'drop') |
There was a problem hiding this comment.
We avoid modifying user params generally.
sklearn/tests/test_pipeline.py
Outdated
| @@ -488,7 +489,7 @@ def test_make_union_kwargs(): | |||
| assert_equal(3, fu.n_jobs) | |||
| # invalid keyword parameters should raise an error message | |||
| assert_raise_message( | |||
| TypeError, | |||
| TypeError, | |||
There was a problem hiding this comment.
This changed indentation seems strange
There was a problem hiding this comment.
Deprecation message reads better now IMO. @jnothman does this need a whatsnew notice?
|
yes, this needs an API Changes what's new notice. thanks for the reminder
Joris
|
|
@jnothman for Pipeline 'passthrough' you argued to for now keep both (not directly deprecate None). Would you then argue the same here, or not? |
|
This PR was updated to not deprecate None. |
Whoops, sorry missed that |
sklearn/tests/test_pipeline.py
Outdated
| @@ -827,7 +827,21 @@ def test_set_feature_union_steps(): | |||
| assert_equal(['mock__x5'], ft.get_feature_names()) | |||
|
|
|||
|
|
|||
| def test_set_feature_union_step_none(): | |||
| def test_set_feature_union_test_none(): | |||
There was a problem hiding this comment.
same comment here as in the other PR, might make sense to parametrize for None / 'drop'
There was a problem hiding this comment.
LGTM, thanks @thomasjpfan
I'll merge this for 0.20 to improve consistency between FeatureUnion and ColumnTransformer
Travis failed to detect it in #11640
Travis failed to detect it in scikit-learn#11640
Reference Issues/PRs
Addresses one of the issues in #11144.
What does this implement/fix? Explain your changes.
Uses
dropandNoneinFeatureUnion.