FIX ColumnTransformer.get_feature_names_out with string slices #22913

randomgeek78 · 2022-03-21T07:32:32Z

The earlier PR (#22775) did not account for string slices. A string slice like slice('col_A', 'col_B') is a valid column specification format and was not handled in the earlier PR. This PR extends the earlier fix to include string slices.

Reference Issues/PRs

Extends Fix #22775

Issue: #22774

What does this implement/fix? Explain your changes.

It extends get_feature_names_out API to include cases where the column specification was using a string slice like slice('col_A', 'col_B')

Any other comments?

randomgeek78 · 2022-03-24T04:59:41Z

How does this PR look? Am I missing something that I could fix? Any suggestions? Thanks!

thomasjpfan

Thanks for the PR!

To be more specific, the whole method can become:

    def _get_feature_name_out_for_transformer(
        self, name, trans, column, feature_names_in
    ):
        """Gets feature names of transformer.

        Used in conjunction with self._iter(fitted=True) in get_feature_names_out.
        """
        if trans == "drop" or _is_empty_column_selection(column):
            return
        elif trans == "passthrough":
            column_indices = self._transformer_to_input_indices[name]
            return feature_names_in[column_indices]

        # An actual transformer
        if not hasattr(trans, "get_feature_names_out"):
            raise AttributeError(
                f"Transformer {name} (type {type(trans).__name__}) does "
                "not provide get_feature_names_out."
            )
        column_indices = self._transformer_to_input_indices[name]
        names = feature_names_in[column_indices]
        return trans.get_feature_names_out(names)

which removes many if statements.

thomasjpfan · 2022-03-24T14:08:46Z

sklearn/compose/tests/test_column_transformer.py

+        (
+            [
+                ("bycol1", TransWithNames(), ["b"]),
+                ("bycol2", "drop", slice("d", "d")),


Can we have a slice that contains more than one element?

thomasjpfan · 2022-03-24T14:15:22Z

sklearn/compose/_column_transformer.py

+                # Assuming column and self._transformer_to_input_indices[name]
+                # are matched. True if column generated from _iter
+                column = self._transformer_to_input_indices[name]
+                return feature_names_in[column]


Looking at this again, I think we can simplify this whole section:

elif trans == "passthrough": column_indices = self._transformer_to_input_indices[name] return feature_names_in[column_indices]

thomasjpfan · 2022-03-24T14:16:34Z

sklearn/compose/_column_transformer.py

@@ -453,6 +459,10 @@ def _get_feature_name_out_for_transformer(
                f"Transformer {name} (type {type(trans).__name__}) does "
                "not provide get_feature_names_out."
            )
+        if isinstance(column, slice) and _determine_key_type(column) == "str":


Same here:

column_indices = self._transformer_to_input_indices[name] names = feature_names_in[column_indices] return trans.get_feature_names_out(names)

and remove the if statements.

@thomasjpfan Thanks for your feedback. When I was writing this, I was not sure if keeping column severed another purpose. Completely depending only on _transformer_to_input_indices will simplify the logic even more as in the latest PR. Do you think the API for _get_feature_name_out_for_transformer shouldn't change? Tx

Since _get_feature_name_out_for_transformer is private, we can change the API if we need to. We need to make sure the test pass for the public get_feature_names_out API.

Although in this case, the column is still used in:

if trans == "drop" or _is_empty_column_selection(column):

I think we can keep this line here for now, so this PR is easier to review. In the future, we can open an follow up PR to refactor _get_feature_name_out_for_transformer.

thomasjpfan · 2022-03-25T19:46:21Z

sklearn/compose/_column_transformer.py

@@ -21,6 +21,7 @@
 from ..utils import Bunch
 from ..utils import _safe_indexing
 from ..utils import _get_column_indices
+from ..utils import _determine_key_type


To fix the linting issue on the CI:

Suggested change

from ..utils import _determine_key_type

Thanks @thomasjpfan for your feedback. I hope I have addressed them.

@thomasjpfan Not sure if this PR is still on your radar or you you want to see it updated in any way. Tx

thomasjpfan

LGTM

randomgeek78 · 2022-03-29T15:55:53Z

@thomasjpfan Thanks. What are the next steps? I am still not able to merge PR. Does policy require another reviewer to review the PR?

thomasjpfan · 2022-03-29T18:35:55Z

Does policy require another reviewer to review the PR?

Yes, this PR requires a second reviewer

randomgeek78 · 2022-03-31T12:55:48Z

@adrinjalali @ogrisel This PR requires another approver. Would it be possible for you to talk a look since it extends the previous PR you approved earlier? Thanks.

adrinjalali

Thanks @randomgeek78

…t-learn#22913) * FIX ColumnTransformer.get_feature_names_out with string slices * Fixed black formatting * Added tests * Updated whats new 1.1 section * Simplified logic and added slices containing two elements * Fixed black linting * Linting cleanup, revert API change * Fixed linting

FIX ColumnTransformer.get_feature_names_out with string slices

f899b81

github-actions bot added the module:compose label Mar 21, 2022

Fixed black formatting

373b6a4

randomgeek78 force-pushed the patch-1 branch from feba1c1 to 373b6a4 Compare March 21, 2022 07:56

randomgeek78 added 2 commits March 21, 2022 00:58

Added tests

59cf8b8

Updated whats new 1.1 section

2dce82b

thomasjpfan reviewed Mar 24, 2022

View reviewed changes

randomgeek78 added 2 commits March 25, 2022 11:44

Simplified logic and added slices containing two elements

119e440

Fixed black linting

eb7f9a2

randomgeek78 force-pushed the patch-1 branch from 72be73f to eb7f9a2 Compare March 25, 2022 19:04

thomasjpfan reviewed Mar 25, 2022

View reviewed changes

randomgeek78 added 2 commits March 25, 2022 12:53

Linting cleanup, revert API change

5dc17f7

Fixed linting

7c28259

thomasjpfan approved these changes Mar 28, 2022

View reviewed changes

adrinjalali approved these changes Mar 31, 2022

View reviewed changes

adrinjalali merged commit 7acb247 into scikit-learn:main Mar 31, 2022

randomgeek78 deleted the patch-1 branch March 31, 2022 13:28

Uh oh!

FIX ColumnTransformer.get_feature_names_out with string slices #22913

FIX ColumnTransformer.get_feature_names_out with string slices #22913

Uh oh!

Conversation

randomgeek78 commented Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

randomgeek78 commented Mar 24, 2022

Uh oh!

thomasjpfan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

randomgeek78 Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

randomgeek78 Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

randomgeek78 Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

randomgeek78 commented Mar 29, 2022

Uh oh!

thomasjpfan commented Mar 29, 2022

Uh oh!

randomgeek78 commented Mar 31, 2022

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

randomgeek78 commented Mar 21, 2022 •

edited

Loading

thomasjpfan left a comment •

edited

Loading

randomgeek78 Mar 25, 2022 •

edited

Loading