change the tuple order in make_column_transformer #12626
Conversation
|
I've left PR #12396 the way it is for us to see both solutions (just in case there's any doubts) and for the reference in case questions come up in the future. |
|
thanks. Whatsnew? |
|
oh yeah, forgot that, on it! |
|
Sweet, thanks! |
| with pytest.warns(DeprecationWarning, | ||
| match='`make_column_transformer` now expects'): | ||
| make_column_transformer(('passthrough', 'passthrough'), | ||
| ('first', 'drop')) |
There was a problem hiding this comment.
Please add a check that the output of fit_transform with this tuple order is the same as the output of fit_transform with the non deprecated tuple order.
| @@ -48,6 +48,10 @@ Changelog | |||
| even if all transformation results are sparse. :issue:`12304` by `Andreas | |||
| Müller`_. | |||
|
|
|||
| - |API| :func:`compose.make_column_transformer` now expects | |||
| `(transformer, columns)` instead of `(columns, transformer)`. | |||
| :issue:`12339` by :user:`Adrin Jalali <adrinjalali>`. | |||
There was a problem hiding this comment.
I would add something like "to be consistent with ColumnTransformer"
| # XXX remove in v0.22 | ||
| with pytest.warns(DeprecationWarning, | ||
| match='`make_column_transformer` now expects'): | ||
| make_column_transformer(('first', scaler)) |
There was a problem hiding this comment.
Should we check here that the resulting ColumnTransformer has the correct order? (or at least once)
|
(Would it hurt if someone else wrapped this up, so that it can be released? I'm guessing @adrinjalali is now afk.) |
| make_column_transformer(('first', scaler)) | ||
| ct1 = make_column_transformer(([0], norm)) | ||
| ct2 = make_column_transformer((norm, [0])) | ||
| X_array = np.array([[0, 1, 2], [2, 4, 6]]).T |
There was a problem hiding this comment.
I think that @jorisvandenbossche meant the correct order of (transformer, column), not the correct order of data... but I suppose this confirms that it's behaving as expected at least.
There was a problem hiding this comment.
and actually I'm addressing @ogrisel's comment here, see #12626 (comment) :)
There was a problem hiding this comment.
Yes, that tests that as well :)
Hmm, but seems that it's not related to the deprecation and we've already have some tests to ensure the correct order (e.g., test_column_transformer_mixed_cols_sparse)? I'll merge this one so that we can release, we can always have some other PRs if needed. |
|
thanks @adrinjalali |
…-learn#12626)" This reverts commit 4eb9133.
…-learn#12626)" This reverts commit 4eb9133.
Fixes #12396
Fixes #12339
Change the expected tuple order in
make_column_transformerto(transformer, columns).