-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[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
[MRG+1] Don't modify steps in Pipeline.__init__ #9716
Conversation
How is this related to #9587 ? Will this change not cause that regression again? |
LGTM @jorisvandenbossche this will be a behaviour regression for users who previously did
But we never really said we'd support tuples and that's a fairly obscure usage pattern. |
@agramfort, I see you've taken to the "approve" button. Should that be understood as LGTM? |
Ah, missed that you do the conversion to list inside Also note that there is such a modification in the |
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 :-/ |
Actually, it does... from sklearn.pipeline import make_union, FeatureUnion
from sklearn.preprocessing import StandardScaler
from sklearn.base import clone
clone(make_union(StandardScaler()))
|
yes indeed :) I am "old" but I can adapt to new features. Just don't make me learn another programming language that's all :) |
Hahaha :)
In other projects I've seen "approve" used to mean "I generally think this
PR is a good idea", not to be a "let's merge it as is". So just checking.
…On 13 September 2017 at 05:37, Alexandre Gramfort ***@***.***> wrote:
@agramfort <https://github.com/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 :)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9716 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz65n3zFwJUHc344OL9HHPaYfoSE1xks5sht2SgaJpZM4PRclj>
.
|
Fixes #9715.
Changing anything in
__init__
will break clone in 0.20 and raises a warning since 0.18.