BUG: force pipeline steps to be list not a tuple #9604
Conversation
|
LGTM +1 for MRG |
sklearn/tests/test_pipeline.py
Outdated
| pipe.fit(X, y=None) | ||
| pipe.score(X) | ||
|
|
||
| X = np.array([[1, 2]]) |
There was a problem hiding this comment.
Not sure what these two lines are for.
|
Otherwise LGTM |
|
Thanks, @jorisvandenbossche |
sklearn/tests/test_pipeline.py
Outdated
| @@ -215,8 +215,6 @@ def test_pipeline_init_tuple(): | |||
| pipe.fit(X, y=None) | |||
| pipe.score(X) | |||
|
|
|||
| X = np.array([[1, 2]]) | |||
| pipe = Pipeline((('transf', Transf()), ('clf', FitParamT()))) | |||
There was a problem hiding this comment.
The X was indeed redundant, but the redefinition of the pipe is actually to make sure that set_params also works when fit is not yet called.
In the end I put my fix (conversion to list) in the __init__ and not in the fit like Alex did, so it shouldn't matter. But now the test is less robust to a change in the Pipeline implementation.
|
I'm not convinced initialising it again reduces much risk.
…On 23 Aug 2017 11:27 pm, "Joris Van den Bossche" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In sklearn/tests/test_pipeline.py
<#9604 (comment)>
:
> @@ -215,8 +215,6 @@ def test_pipeline_init_tuple():
pipe.fit(X, y=None)
pipe.score(X)
- X = np.array([[1, 2]])
- pipe = Pipeline((('transf', Transf()), ('clf', FitParamT())))
The X was indeed redundant, but the redefinition of the pipe is actually
to make sure that set_params also works when fit is not yet called.
In the end I put my fix (conversion to list) in the __init__ and not in
the fit like Alex did, so it shouldn't matter. But now the test is less
robust to a change in the Pipeline implementation.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#9604 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz64iM6VdMzrpdOCbQAgai0Jt02Q4Cks5sbCjVgaJpZM4O-wuL>
.
|
|
If someone would move the conversion of steps to a list from the init to the fit method (as eg validation of parameters often happens in the fit and not init), then |
|
feel free to change it, but comment clearly what the purpose of the test
is. tests need to be readable
On 24 Aug 2017 7:48 am, "Joris Van den Bossche" <notifications@github.com> wrote:
If someone would move the conversion of steps to a list from the init to
the fit method (as eg validation of parameters often happens in the fit and
not init), then set_params will be broken in the specific case that the
pipeline was not yet fitted before, and the current test will not catch
that.
(that's what I meant with "less robust to a change in the Pipeline
implementation.")
But given that is not that likely we will change that (as an actual
refactor of Pipeline would ensure steps is not mutated, and doesn't need to
be a list), this is not that important :-)
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#9604 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6wrzMO65HExwkQZfycNvU2RBV9omks5sbJ4lgaJpZM4O-wuL>
.
|
Alternative to #9221. Combined the fix of @agramfort and suggestion of @jnothman. And added a more explicit test for it.
Fixes #9587, closes #9221
What does this implement/fix? Explain your changes.
Previously passing a tuple as the steps to a Pipeline worked, this broke in 0.19.
Therefore, this PR converts the passed steps to a list.
This is modifying an init argument (
self.steps). However, there is currently a problem with the Pipeline implementation that the fitted steps are saved inself.stepsand not inself.steps_. Therefore,self.stepsneeds to be mutable and cannot be a tuple.The correct fix would be to solve the design problem, for which there is a PR (#8350). This PR provides a smaller temporary fix to undo the regression, until the other PR is merged (which will certainly not be in a bugfix release)