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

[MRG] DEP Deprecate None in FeatureUnion #15053

Merged

Conversation

@thomasjpfan
Copy link
Member

thomasjpfan commented Sep 21, 2019

Reference Issues/PRs

Related to #14813

What does this implement/fix? Explain your changes.

Deprecates None in FeatureUnion.

Any other comments?

Two options to do the same thing is undesirable.

thomasjpfan added 5 commits Sep 21, 2019
STY
Copy link
Member

adrinjalali left a comment

Also needs a whats_new entry

@@ -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

This comment has been minimized.

Copy link
@adrinjalali

adrinjalali Sep 23, 2019

Member

drop or None?

@pytest.mark.parametrize('drop', ['drop', None])
@ignore_warnings(category=DeprecationWarning)

This comment has been minimized.

Copy link
@adrinjalali

adrinjalali Sep 23, 2019

Member

shouldn't we fix the test instead?

thomasjpfan added 3 commits Sep 23, 2019
Copy link
Member

adrinjalali left a comment

LGTM, thanks @thomasjpfan

Copy link
Contributor

NicolasHug left a comment

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 Show resolved Hide resolved


# TODO: Remove in 0.24 when None is removed
def test_feature_union_warns_with_drop():

This comment has been minimized.

Copy link
@NicolasHug

NicolasHug Sep 25, 2019

Contributor
Suggested change
def test_feature_union_warns_with_drop():
def test_feature_union_warns_with_none():
@@ -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

This comment has been minimized.

Copy link
@NicolasHug

NicolasHug Sep 25, 2019

Contributor
Suggested change
# 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!)

@rth
rth approved these changes Oct 4, 2019
Copy link
Member

rth left a comment

Please fix merge conflicts. LGTM, otherwise.

…ature_union
@adrinjalali

This comment has been minimized.

Copy link
Member

adrinjalali commented Oct 4, 2019

linter error. Otherwise LGTM

@adrinjalali adrinjalali merged commit 16f4ac9 into scikit-learn:master Oct 6, 2019
19 checks passed
19 checks passed
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.84%)
Details
codecov/project 96.84% (+<.01%) compared to a47e914
Details
scikit-learn.scikit-learn Build #20191004.22 succeeded
Details
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas succeeded
Details
scikit-learn.scikit-learn (Linux py35_ubuntu_atlas) Linux py35_ubuntu_atlas succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_conda_mkl) Linux pylatest_conda_mkl succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
scikit-learn.scikit-learn (Linux32 py35_ubuntu_atlas_32bit) Linux32 py35_ubuntu_atlas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py35_pip_openblas_32bit) Windows py35_pip_openblas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
scikit-learn.scikit-learn (macOS pylatest_conda_mkl) macOS pylatest_conda_mkl succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.