Skip to content

Conversation

@thomasjpfan
Copy link
Member

Reference Issues/PRs

Closes #24906

What does this implement/fix? Explain your changes.

This PR adds __getitem__ to FeatureUnion to access transformers.

Copy link
Contributor

@Micky774 Micky774 left a comment

Choose a reason for hiding this comment

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

PR looks good modulo the test modification.

@betatim
Copy link
Member

betatim commented Dec 5, 2022

The __getitem__ of Pipeline supports slicing, which then returns a Pipeline containing only a subset of the steps. Worth implementing this here as well? If not, should we check the type of the argument to give an useful error message??

@thomasjpfan
Copy link
Member Author

I do not think FeatureUnion.__getitem__ should support slicing or integers. I updated this PR to restrict the key to strings.

Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

LGTM.

Fine with me not to support slicing. Can you write down a short summary of the thought process so people from the future (and for my education) can find out?

@thomasjpfan
Copy link
Member Author

Fine with me not to support slicing. Can you write down a short summary of the thought process so people from the future (and for my education) can find out?

For me, I do not see a good user story for adding slices to FeatureUnion. For Pipeline, it is relatively common to do pipeline[:-1] to return a pipeline with only the transformers.

Logistically, I prefer to have a more restrictive feature and only consider expanding it when there is a need.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@jeremiedbb jeremiedbb merged commit f7e6977 into scikit-learn:main Dec 6, 2022
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.

Add __get_item__() to FeatureUnion

4 participants