Fix - `get_feature_names_out` fails in two cases by rcap107 · Pull Request #1543 · skrub-data/skrub

rcap107 · 2025-07-23T20:52:47Z

rcap107 · 2025-07-28T13:32:48Z

The tests are failing with the min-optional-deps configuration because the column transformer in min-deps configuration does not support polars dataframes: in all_columns, it is expecting a pandas index, while polars gives a list.

How should I deal with this ? I can remove the test with the column transformer (I added it mostly to check for the behavior in #1540), but maybe there is a way to skip the test for the given configuration. What's the best choice here?

Vincent-Maladiere

Hey @rcap107! After testing on different CI versions with pixi, it looks that the error you have is specific to Polars on column transformers, on a prior version of sklearn. We can go two ways:

We force conversion from polars to Pandas for old sklearn versions
or we ignore it and suggest people to upgrade to latest sklearn and polars if they aim at using column transformer

WDYT?

Co-authored-by: Vincent M <maladiere.vincent@yahoo.fr>

rcap107 · 2025-07-29T12:32:04Z

Hey @rcap107! After testing on different CI versions with pixi, it looks that the error you have is specific to Polars on column transformers, on a prior version of sklearn. We can go two ways:

We force conversion from polars to Pandas for old sklearn versions

or we ignore it and suggest people to upgrade to latest sklearn and polars if they aim at using column transformer

WDYT?

Personally I'd rather go with the second, but since it's on sklearn side I wonder if someone that is stuck on that version just can't upgrade it 🤔

So I think the first choice is overall better, but we need to document it properly

rcap107 · 2025-07-30T08:23:48Z

Hey @rcap107! After testing on different CI versions with pixi, it looks that the error you have is specific to Polars on column transformers, on a prior version of sklearn. We can go two ways:

We force conversion from polars to Pandas for old sklearn versions

or we ignore it and suggest people to upgrade to latest sklearn and polars if they aim at using column transformer

WDYT?

Personally I'd rather go with the second, but since it's on sklearn side I wonder if someone that is stuck on that version just can't upgrade it 🤔

So I think the first choice is overall better, but we need to document it properly

I'm trying to implement this, but it's not clear to me where I should force the conversion to Polars, given that the call that fails is inside the code of the column transformer, and not on our side

Unless we convert by default (again, where?) polars dataframes to pandas if the sklearn version is too low.

Vincent-Maladiere · 2025-07-30T09:13:49Z

You're right, the issue is on sklearn side. One way to do it would be to patch the fit_transform method of the ColumnTransformer for sklearn < 1.?.0. in skrub/sklearn_compat.py. Or we could ignore it and skip fail the test in this situation.

rcap107 · 2025-08-01T09:55:08Z

I had to patch the fit_transform and transform methods of the ColumnTransformer so that they convert X to pandas if it is a polars dataframe or series

Vincent-Maladiere

Hey @rcap107, some feedback

Vincent-Maladiere · 2025-08-01T11:47:12Z

+        ColumnTransformer.original_fit_transform = ColumnTransformer.fit_transform
+        ColumnTransformer.original_transform = ColumnTransformer.transform
+
+        def _patched_fit_transform(self, X, y=None):


We could simplify the logic slightly by using a decorator, and use it like:

ColumnTransformer.fit_transform = wrap_to_pandas(ColumnTransformer.fit_transform) ColumnTransformer.transform = wrap_to_pandas(ColumnTransformer.transform)

Vincent-Maladiere · 2025-08-01T11:47:27Z

+            original_type = None
+            if isinstance(X, (pl.DataFrame, pl.Series)):
+                original_type = type(X)
+                X = X.to_pandas()


what happens if y is not None?

y is forwarded unchanged to the ColumnTransformer in the fit_transform, transform does not take y at all

Would it be an issue if y was a pandas DataFrame/series?

rcap107 · 2025-08-01T13:58:44Z

as it turns out, the bug with DropCols is also affecting any situation where ApplyToCols is used, both explicitly, or from within the dataops evaluation, so it's a more serious issue than I had first understood

Vincent-Maladiere · 2025-08-01T15:56:58Z

the bug with DropCols is also affecting any situation where ApplyToCols is used

I'm not sure to follow, could you show an example?

rcap107 · 2025-08-01T16:58:18Z

the bug with DropCols is also affecting any situation where ApplyToCols is used

I'm not sure to follow, could you show an example?

this code fails on main, but it works with the fix in this PR

#%%
import skrub
from skrub import DropCols
import polars as pl
import skrub.selectors as s
from skrub._apply_to_cols import ApplyToCols

df = pl.DataFrame({
    "a": [1, 2, 3],
    "b": [4, 5, 6],})

# %%
app = ApplyToCols(DropCols(s.glob("")),)
app.fit_transform(df)
# %%
v = skrub.var("df", df)
v.skb.apply(app)

This reverts commit e40242b.

rcap107 · 2025-08-04T11:46:56Z

The reason this issue is stuck is because the ColumnTransformer does not work with polars on scikit-learn < 1.5, so I tried to patch it to fix the problem. However, this was a bigger issue than anticipated.

I realized that the test that was failing was something that I added to reproduce the error in #1540, so the problem has been there for much longer.

I think we should ignore that particular problem in this PR, and tackle separately. Here, we can address the specific problems with #1540.

rcap107 · 2025-08-04T11:49:22Z

In short, I'll remove the extra test that is failing and all the sklearn compat code, and continue with the rest so we can merge the fix

…1540

glemaitre

It looks a similar fix than in #1256 so LGTM.

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

Co-authored-by: Jérôme Dockès <jerome@dockes.org> Co-authored-by: Vincent M <maladiere.vincent@yahoo.fr> Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

Fix 1540

4b0974f

rcap107 changed the title ~~Fix 1540~~ Fix - get_feature_names_out fails in two cases Jul 24, 2025

rcap107 added 3 commits July 28, 2025 13:42

Merge remote-tracking branch 'upstream/main' into fix-1540

2ccc63f

fixing gap

eab698b

fix dropcols

f1cb191

rcap107 marked this pull request as ready for review July 28, 2025 12:46

rcap107 added 2 commits July 28, 2025 15:01

_

bfef78c

_

a55f11a

rcap107 added this to the 0.6.1 milestone Jul 28, 2025

check if missing method exists in .skb namespace (skrub-data#1541)

693bff1

Vincent-Maladiere reviewed Jul 29, 2025

View reviewed changes

Comment thread CHANGES.rst Outdated

Comment thread skrub/tests/test_select_cols.py Outdated

rcap107 and others added 2 commits July 29, 2025 14:28

Update CHANGES.rst

d4753f6

Co-authored-by: Vincent M <maladiere.vincent@yahoo.fr>

Update skrub/tests/test_select_cols.py

61f2b16

Co-authored-by: Vincent M <maladiere.vincent@yahoo.fr>

rcap107 added 4 commits August 1, 2025 09:50

monkey patching compatibility

e184cd2

_

8dc8aff

_

4a3afb3

fix

77b99c8

Vincent-Maladiere reviewed Aug 1, 2025

View reviewed changes

rcap107 added 2 commits August 1, 2025 14:02

adding decorator

7852415

workaround

1ddbea7

rcap107 added 2 commits August 4, 2025 12:19

adding auto_wrap_output_keys to classes, removing compat

e40242b

Revert "adding auto_wrap_output_keys to classes, removing compat"

f9fff55

This reverts commit e40242b.

removing offending test and skcompat stuff

e213c35

rcap107 mentioned this pull request Aug 8, 2025

BUG - ColumnTransformer fails with min-optional-reqs when using a Polars dataframe #1557

Closed

rcap107 added 2 commits August 10, 2025 11:47

Fix 1540

a7dbc68

Merge branch 'fix-1540' of https://github.com/rcap107/skrub into fix-…

0d141e8

…1540

glemaitre reviewed Aug 11, 2025

View reviewed changes

Comment thread skrub/tests/test_select_cols.py

glemaitre approved these changes Aug 11, 2025

View reviewed changes

rcap107 and others added 2 commits August 11, 2025 20:39

Update skrub/tests/test_select_cols.py

ab0d01d

Co-authored-by: Guillaume Lemaitre <guillaume@probabl.ai>

rebooting ci

fb989e3

rcap107 merged commit 73053a0 into skrub-data:main Aug 12, 2025
26 checks passed

rcap107 deleted the fix-1540 branch August 12, 2025 11:42

rcap107 mentioned this pull request Sep 19, 2025

DatetimeEncoder.get_feature_names_out fails because of the wrong inheritance #1621

Closed

Uh oh!

Conversation

rcap107 commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rcap107 commented Jul 28, 2025

Uh oh!

Vincent-Maladiere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rcap107 commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rcap107 commented Jul 30, 2025

Uh oh!

Vincent-Maladiere commented Jul 30, 2025

Uh oh!

rcap107 commented Aug 1, 2025

Uh oh!

Vincent-Maladiere left a comment

Choose a reason for hiding this comment

Uh oh!

Vincent-Maladiere Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

Vincent-Maladiere Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

rcap107 Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

Vincent-Maladiere Aug 1, 2025 • 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!

rcap107 commented Aug 1, 2025

Uh oh!

Vincent-Maladiere commented Aug 1, 2025

Uh oh!

rcap107 commented Aug 1, 2025

Uh oh!

rcap107 commented Aug 4, 2025

Uh oh!

rcap107 commented Aug 4, 2025

Uh oh!

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rcap107 commented Jul 23, 2025 •

edited

Loading

rcap107 commented Jul 29, 2025 •

edited

Loading

Vincent-Maladiere Aug 1, 2025 •

edited

Loading