Skip to content

MAINT refactoring get_feature_names_out to define it in encoders#1634

Merged
rcap107 merged 4 commits into
skrub-data:mainfrom
rcap107:refactor-get_feature_names_out
Sep 25, 2025
Merged

MAINT refactoring get_feature_names_out to define it in encoders#1634
rcap107 merged 4 commits into
skrub-data:mainfrom
rcap107:refactor-get_feature_names_out

Conversation

@rcap107

@rcap107 rcap107 commented Sep 25, 2025

Copy link
Copy Markdown
Member

Remove get_feature_names_out from the SingleColumnTransformer class and add it to the specialized encoders since they might specialized depending on their own parameters.

@rcap107 rcap107 marked this pull request as ready for review September 25, 2025 08:30
@rcap107

rcap107 commented Sep 25, 2025

Copy link
Copy Markdown
Member Author

This is the first part of an issue on refactoring the get_feature_names_out function. Here I moved it out of the SingleColumnTransformer, later we might want to create a class that abstracts it. We can't use the SingleColumnTransformer for that because not all the encoders that inherit from this use the same get_feature_names_out

@glemaitre glemaitre changed the title refactoring get_feature_names_out MAINT refactoring get_feature_names_out to define it in encoders Sep 25, 2025
@glemaitre glemaitre self-requested a review September 25, 2025 09:03

@glemaitre glemaitre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As a first step, it looks good. We can further see if we want to limit the code redundancy but since the remaining duplicated code are in different files, I'm not sure that in terms of maintenance and code complexity, it is better to get an abstraction just for the get_feature_names_out and encoder exposing a n_components_ attribute.

@glemaitre glemaitre self-requested a review September 25, 2025 09:36

@glemaitre glemaitre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just pushed a small fixed to make sure that _SplineEncoder follows the scikit-learn API (not overwrite attributes) and make sure that we cover the line that was not covered before.

@rcap107 rcap107 merged commit d886018 into skrub-data:main Sep 25, 2025
26 checks passed
@rcap107 rcap107 deleted the refactor-get_feature_names_out branch September 25, 2025 12:24
rcap107 added a commit to rcap107/skrub that referenced this pull request Sep 25, 2025
…ub-data#1634)

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>
rcap107 added a commit that referenced this pull request Sep 25, 2025
Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>
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.

2 participants