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

force pipeline steps to be list not a tuple #9221

Closed
wants to merge 1 commit into from

Conversation

agramfort
Copy link
Member

@agramfort agramfort commented Jun 25, 2017

as pipeline needs to support assignment

Fixes #9587

@@ -145,6 +145,9 @@ def set_params(self, **kwargs):
return self

def _validate_steps(self):
if isinstance(self.steps, tuple):
self.steps = list(self.steps)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modifying an init argument :(

Copy link
Member Author

@agramfort agramfort Jun 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you suggest?

it is modified anyway in : L226

self.steps[step_idx] = (name, fitted_transformer)

which is what created the problem

@jnothman
Copy link
Member

Is the idea here to support set_params(step_name=blah). Changing this line to

        new_estimators = list(getattr(self, attr))

should work for other implementations of _BaseComposition

@agramfort
Copy link
Member Author

@jnothman I don't follow what you mean. Please send me a PR or take over. thx

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Jun 27, 2017

The problem with assigning the fitted transformer into the self.steps should be solved after #8350 (but assigning a new transformer in set_params will still raise an error)

The suggestion of @jnothman is also a nice way to fix it (and indeed fixes it for all subclass of BaseComposition), but it has the disadvantage of self.steps changing from tuple to list only after certain usage depending on what the user did.

But maybe in general, do we actually want to allow tuples? (if so, probably need to add a test for it)

@jnothman
Copy link
Member

I didn't realise this PR was fixing a regression, @agramfort, as #9587 shows this is.

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

Successfully merging this pull request may close these issues.

None yet

4 participants