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

ENH Adds FeatureUnion.__getitem__ to access transformers #25093

Merged
merged 5 commits into from
Dec 6, 2022

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.

Co-authored-by: Meekail Zain <34613774+Micky774@users.noreply.github.com>
@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