API ColumnTransformer.transformers_: passthrough -> FunctionTransformer #27204

adrinjalali · 2023-08-29T16:50:30Z

While working on adding metadata routing to ColumnTransformer, I was quite confused by certain parts of the code. This PR adds a few docstrings to help future poor souls reading the code.

With @glemaitre we also noticed "passthrough" is kept as is instead of being replaced by the corresponding FunctionTransformer in .transformers_. Putting the fitted FunctionTransformer in the fitted transformers_ attribute would make sense, and it simplifies the code quite a bit.

Some other parts are also cleaned up as a result, and also discovered a bug which was detected once "passthrough" was replaced (and our tests detected the bug).

._iter(...) now has more explicit args to filter steps, and there's no more a need for replace_strings argument.

cc @thomasjpfan

…lean

github-actions · 2023-08-29T16:52:49Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 70f6e68. Link to the linter CI: here}

lorentzenchr

LGTM

sklearn/compose/_column_transformer.py

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>

thomasjpfan

Thanks for the PR! The title needs to change to a API tag, because ColumnTransformer's API changes.

thomasjpfan · 2023-09-02T11:48:37Z

doc/whats_new/v1.4.rst

+- |API| :class:`~compose.ColumnTransformer` now replaces `"passthrough"` with a
+  corresponding :class:`~preprocessing.FunctionTransformer` in the fitted


I always wanted this change, but it is backward incompatible. I am undecided on going through a deprecation cycle.

This is a FIX to me, and we really won't be breaking many people's code by this change.

I find the backward incompatibility problematic. I would have considered as a fix if we would have only transformers in transformers_ and no string. But this is not the case. I can imagine some user code relying on the "passthrough" or "drop" strings. I know that this is annoying to handle those but I would rather be more confortable with a proper deprecation cycle.

I can imagine that we could a keyword legacy_mode_attributes that could allow replace back the FunctionTransformer in the transformers_ property and raise the deprecation warnings.

That would mean introducing the arg, do the deprecation, then introduce a deprecation for the arg that we just added to deprecate something which should never have been there in the first place. I rather not do that and annoy very few people whose code might break.

Is there any Python trick that would make string comparisons FunctionTransformer() == 'passthrough' evaluate to True, but keep repr as intended?

I see @lorentzenchr and yourself OK to consider it as a bug fix. So let's go with it.

I just summed up the net benefit of both paths und the bugfix version won with less estimated trouble in total/for everyone.

It is what it is, but I'm going to throw in the opinion that this should have gone through a deprecation cycle. I have a production pipeline containing a very long-running incremental estimator that can't predict on 1.4.0, and is nontrivial to retrain from scratch.

Note that we generally do not guarantee any form of forward compatibility of trained estimators. So train with 1.3 and load and predict with 1.4 won't work (or just by chance).
The pipeline itself that you have is very likely to work with this PR merged as long as you don't directly access the transformers_ attribute of a ColumnTransformer.

Yes, though perhaps by coincidence there's previously been enough compatibility such that I've been able to decouple our dependency upgrades and re-fitting our stateful transformers to prevent service downtime. This update threw the following error in our test suite (due to "pass through")

> res = transformer.transform(X, **params.transform) E AttributeError: 'str' object has no attribute 'transform' ../../../../.cache/pypoetry/virtualenvs/pricingbanditservice-pioIYUID-py3.11/lib/python3.11/site-packages/sklearn/pipeline.py:1283: AttributeError

Fortunately, the update is backward compatible enough, so the solution for us was to train a new pipeline on 1.4.0, start serving it on 1.3.2, then do the dependency update.

betatim · 2023-09-05T13:03:30Z

sklearn/compose/_column_transformer.py

-        of tuples of len 2.
+        transformers, dropping the columns.
+
+        DO NOT USE: This is for the implementation of get_params via


nit: Why the loud shouty "DO NOT USE"? Can we turn it into something more informative? Otherwise I feel like I didn't learn anything that I didn't already know from _transformers being a private attribute.

Extended the comment.

This is not rendered in our docs, so the note is not meant for end users. This is for us and future poor maintainers who will be confused. I started by using this instead of self._iter to iterate through the transformers and then I realized I shouldn't.

…lean

…it-learn into columntransformer/clean

sklearn/compose/_column_transformer.py

avm19 · 2023-09-13T16:19:00Z

sklearn/compose/_column_transformer.py

+        if func is _fit_transform_one:
+            fitted = False
+        else:  # func is _transform_one
+            fitted = True
+


There are two reasons to keep fitted as a separate parameter: (i) explicit is better than implicit; (ii) potentially when partial_fit is implemented we'll need fitted=False for the first call and fitted=True for subsequent calls.

this was as a result of a previous review, and I rather keep it as is now.

…lean

adrinjalali · 2023-09-17T13:21:56Z

Should we merge this then?

lorentzenchr · 2023-09-17T19:24:14Z

Should we merge this then?

We need a second reviewer's approval, don't we?

glemaitre · 2023-09-18T08:06:49Z

Let me review this one. I'll do once I branch for the release 1.3.1

glemaitre

Apart of making the deprecation explicit, I am fine with the code changes.

sklearn/compose/_column_transformer.py

glemaitre · 2023-09-25T13:04:32Z

sklearn/compose/_column_transformer.py

+            If True, 'drop' transformers are filtered out.
+
+        skip_empty_columns : bool
+            If True, transformers with empty selected columns are filtered out.


Could we also have Yields section to show that this is a generator.

I tried adding something, not sure if you like it.

Yep this is fine with me.

glemaitre · 2023-09-25T13:16:42Z

doc/whats_new/v1.4.rst

+- |API| :class:`~compose.ColumnTransformer` now replaces `"passthrough"` with a
+  corresponding :class:`~preprocessing.FunctionTransformer` in the fitted


I find the backward incompatibility problematic. I would have considered as a fix if we would have only transformers in transformers_ and no string. But this is not the case. I can imagine some user code relying on the "passthrough" or "drop" strings. I know that this is annoying to handle those but I would rather be more confortable with a proper deprecation cycle.

I can imagine that we could a keyword legacy_mode_attributes that could allow replace back the FunctionTransformer in the transformers_ property and raise the deprecation warnings.

…lean

glemaitre · 2023-10-05T09:45:32Z

@adrinjalali Could you change make the entry as a |Fix| (even if we have both tags.

…lean

glemaitre

LGTM

glemaitre · 2023-10-05T10:01:08Z

Adding auto-merge here.

…er (scikit-learn#27204) Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>

adrinjalali added 4 commits August 3, 2023 13:40

starting

3badcd1

Merge remote-tracking branch 'upstream/main' into columntransformer/c…

7ef9c37

…lean

tests pass

04094a9

more docs

1091d36

github-actions bot added the module:compose label Aug 29, 2023

add changelog

ffab736

glemaitre self-requested a review August 29, 2023 16:58

Merge branch 'main' into columntransformer/clean

d6ec337

adrinjalali mentioned this pull request Aug 31, 2023

ENH Adds polars output support to ColumnTransformer #26683

Merged

lorentzenchr approved these changes Aug 31, 2023

View reviewed changes

sklearn/compose/_column_transformer.py Outdated Show resolved Hide resolved

sklearn/compose/_column_transformer.py Outdated Show resolved Hide resolved

adrinjalali and others added 2 commits September 1, 2023 03:00

Update sklearn/compose/_column_transformer.py

9532ad1

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>

Update sklearn/compose/_column_transformer.py

22d1fe2

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>

thomasjpfan reviewed Sep 2, 2023

View reviewed changes

adrinjalali changed the title ~~MNT some ColumnTransformer cleanup and add a few docstrings~~ API some ColumnTransformer cleanup and add a few docstrings Sep 2, 2023

adrinjalali changed the title ~~API some ColumnTransformer cleanup and add a few docstrings~~ API ColumnTransformer.transformers_: passtgrough -> FunctionTransformer Sep 2, 2023

thomasjpfan changed the title ~~API ColumnTransformer.transformers_: passtgrough -> FunctionTransformer~~ API ColumnTransformer.transformers_: passthrough -> FunctionTransformer Sep 3, 2023

betatim reviewed Sep 5, 2023

View reviewed changes

adrinjalali added 3 commits September 6, 2023 12:53

DOC extend _transformers comment

3dc0f66

Merge remote-tracking branch 'upstream/main' into columntransformer/c…

69c2b27

…lean

Merge branch 'columntransformer/clean' of github.com:adrinjalali/scik…

dc6c116

…it-learn into columntransformer/clean

avm19 reviewed Sep 13, 2023

View reviewed changes

sklearn/compose/_column_transformer.py Outdated Show resolved Hide resolved

avm19 reviewed Sep 13, 2023

View reviewed changes

adrinjalali added 2 commits September 17, 2023 15:20

_iter param renames

57accb7

Merge remote-tracking branch 'upstream/main' into columntransformer/c…

dfad040

…lean

glemaitre approved these changes Sep 25, 2023

View reviewed changes

glemaitre self-requested a review September 25, 2023 13:17

glemaitre requested review from glemaitre and removed request for glemaitre September 25, 2023 13:17

adrinjalali added 2 commits September 25, 2023 17:50

improve docs

80cbeb2

Merge remote-tracking branch 'upstream/main' into columntransformer/c…

479f263

…lean

glemaitre mentioned this pull request Oct 4, 2023

Better inference of the columns remainder dtype in transformers_ from ColumnTransformer #27533

Closed

adrinjalali added 2 commits October 5, 2023 11:50

fix changelog

351dd3b

Merge remote-tracking branch 'upstream/main' into columntransformer/c…

70f6e68

…lean

glemaitre approved these changes Oct 5, 2023

View reviewed changes

glemaitre enabled auto-merge (squash) October 5, 2023 10:01

glemaitre merged commit 24b782f into scikit-learn:main Oct 5, 2023

adrinjalali deleted the columntransformer/clean branch October 5, 2023 11:47

REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023

API ColumnTransformer.transformers_: passthrough -> FunctionTransform…

371ad2b

…er (scikit-learn#27204) Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>

ksaur mentioned this pull request Jan 19, 2024

SKL 1.4 release breaks build microsoft/hummingbird#755

Closed

mvdoc mentioned this pull request Mar 28, 2024

FIX ColumnKernelizer for newer sklearn versions gallantlab/himalaya#54

Merged

		- \|API\| :class:`~compose.ColumnTransformer` now replaces `"passthrough"` with a
		corresponding :class:`~preprocessing.FunctionTransformer` in the fitted

Uh oh!

API ColumnTransformer.transformers_: passthrough -> FunctionTransformer #27204

API ColumnTransformer.transformers_: passthrough -> FunctionTransformer #27204

Uh oh!

Conversation

adrinjalali commented Aug 29, 2023

Uh oh!

github-actions bot commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

lorentzenchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

betatim Sep 5, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented Sep 17, 2023

Uh oh!

lorentzenchr commented Sep 17, 2023

Uh oh!

glemaitre commented Sep 18, 2023

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Oct 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Oct 5, 2023

Uh oh!

Uh oh!

github-actions bot commented Aug 29, 2023 •

edited

Loading

betatim Sep 5, 2023 •

edited

Loading

glemaitre commented Oct 5, 2023 •

edited

Loading