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

Update index handling in PandasAdapter #28731

Open
it176131 opened this issue Mar 31, 2024 · 4 comments
Open

Update index handling in PandasAdapter #28731

it176131 opened this issue Mar 31, 2024 · 4 comments

Comments

@it176131
Copy link
Contributor

it176131 commented Mar 31, 2024

Describe the workflow you want to enable

As noted in #27037, handling the index of an input container can be hairy. The solution implemented in #27044 works, but it excludes pandas.Series input types. I'd like to modify the logic in the :method:PandasAdapter.create_container so that it checks if the X_original is a pandas.DataFrame or pandas.Series. This would allow transformers that accept 1-dimensional inputs and output 2-dimensional dataframes to persist their indices.

Describe your proposed solution

I'd like to change line 124 from this:

            elif isinstance(X_original, pd.DataFrame):

To this:

            elif isinstance(X_original, (pd.DataFrame, pd.Series)):

Describe alternatives you've considered, if relevant

User sets the index on their own:

some_series = pd.Series(...)
trf = SomeTransformer().set_output(transform="pandas")
out_frame = trf.fit_transform(some_series).set_index(some_series.index)

Additional context

I recognize most transformers in scikit-learn expect 2-dimensional inputs. But some packages that depend on scikit-learn (like mlxtend) have transformers that transform 1-dimensional input into 2-dimensional output. I believe this would greatly benefit them. See the newly updated TransactionEncoder for an example.

I'm willing to submit a PR if this is an acceptable enhancement.

@it176131 it176131 added Needs Triage Issue requires triage New Feature labels Mar 31, 2024
@glemaitre
Copy link
Member

This is currently outside our API scope and I don't think that we want to make some code that we are not using internally. I think this is a design purpose that scikit-learn define X to be a 2D container because it remove some ambiguity regarding features/samples.

We might be revisit this if we come to think of transformer that make independent column-wise transform.

So I'm -1 for the inclusion right now.

@glemaitre glemaitre removed the Needs Triage Issue requires triage label Apr 1, 2024
@it176131
Copy link
Contributor Author

it176131 commented Apr 1, 2024

This is currently outside our API scope and I don't think that we want to make some code that we are not using internally. I think this is a design purpose that scikit-learn define X to be a 2D container because it remove some ambiguity regarding features/samples.

We might be revisit this if we come to think of transformer that make independent column-wise transform.

So I'm -1 for the inclusion right now.

Makes sense. Perhaps this would be useful for the CountVectorizer, TfidfVectorizer, and TfidfTransformer as they expect a 1D container? Of course, they output sparse matrices by default, but maybe an option in the future.

@glemaitre
Copy link
Member

they expect a 1D container?

As input but not as output. Here, I think that this is more on the output side that you want to act.

@it176131
Copy link
Contributor Author

it176131 commented Apr 1, 2024

they expect a 1D container?

As input but not as output. Here, I think that this is more on the output side that you want to act.

Correct—those transformers take 1D input and then output a 2D object. I figure if they had access to the set_output method and the user chose to return a dense object rather than sparse, we'd want to persist the index.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants