Skip to content

[MRG+1] Don't modify steps in Pipeline.__init__ #9716

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

Merged
merged 5 commits into from
Sep 12, 2017

Conversation

amueller
Copy link
Member

@amueller amueller commented Sep 8, 2017

Fixes #9715.

Changing anything in __init__ will break clone in 0.20 and raises a warning since 0.18.

@amueller amueller changed the title Don't modify steps in Pipeline.__init__ [MRG] Don't modify steps in Pipeline.__init__ Sep 8, 2017
@amueller amueller added this to the 0.19.1 milestone Sep 11, 2017
@jorisvandenbossche
Copy link
Member

How is this related to #9587 ? Will this change not cause that regression again?

@amueller
Copy link
Member Author

#9604 has a regression test, right? And that passes. It's related to #9587 in that it undoes the API violation from #9604 ;)

@jnothman
Copy link
Member

LGTM

@jorisvandenbossche this will be a behaviour regression for users who previously did

p = Pipeline((('a', Something()),))
p.steps[0] = ('a', SomeOtherThing())

But we never really said we'd support tuples and that's a fairly obscure usage pattern.

@jnothman
Copy link
Member

@agramfort, I see you've taken to the "approve" button. Should that be understood as LGTM?

@jnothman jnothman changed the title [MRG] Don't modify steps in Pipeline.__init__ [MRG+1] Don't modify steps in Pipeline.__init__ Sep 12, 2017
@jorisvandenbossche
Copy link
Member

Ah, missed that you do the conversion to list inside fit. So that indeed passes the test I added (although the test should actually be changed slightly to re-initialize pipe in test_pipeline_init_tuple for the set_params test, to make sure set_params works before the pipe is fitted).
Note that the API violation already existed before #9604 :-) (depending on the type of the steps arg)

Also note that there is such a modification in the __init__ of FeatureUnion as well.

@amueller
Copy link
Member Author

Well before #9604 there was no deprecation warning in the tests. I'm not sure in which case a copy or conversion happened. I'm not sure why FeatureUnion doesn't raise a warning :-/

@amueller
Copy link
Member Author

amueller commented Sep 12, 2017

Actually, it does...

from sklearn.pipeline import make_union, FeatureUnion
from sklearn.preprocessing import StandardScaler
from sklearn.base import clone
clone(make_union(StandardScaler()))

base.py:115: DeprecationWarning: Estimator FeatureUnion modifies parameters in init. This behavior is deprecated as of 0.18 and support for this behavior will be removed in 0.20.

@agramfort
Copy link
Member

@agramfort, I see you've taken to the "approve" button. Should that be understood as LGTM?

yes indeed :)

I am "old" but I can adapt to new features. Just don't make me learn another programming language that's all :)

@jnothman
Copy link
Member

jnothman commented Sep 12, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants