Skip to content

Conversation

@agramfort
Copy link
Member

@agramfort agramfort commented Jun 25, 2017

as pipeline needs to support assignment

Fixes #9587


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.

4 participants