ENH add metadata routing to ColumnTransformer #27005
Conversation
There was a problem hiding this comment.
Thanks for the PR @adrinjalali . Here are a few comments considering files other than the tests.
| @@ -591,7 +613,9 @@ def _update_fitted_transformers(self, transformers): | |||
| transformers_ = [] | |||
| self._name_to_fitted_passthrough = {} | |||
|
|
|||
| for name, old, column, _ in self._iter(): | |||
| for name, old, column, _ in self._iter( | |||
There was a problem hiding this comment.
Since this function was complex to understand, do you mind putting a docstring (at least a summary).
There was a problem hiding this comment.
I have another branch where I'm doing more docstrings and some refactoring, adding the docstring there. Wanted to keep this PR somewhat small.
| _transform_one. | ||
|
|
||
| fitted : bool | ||
| Used to get an iterable of transformers. If True, use the fitted |
There was a problem hiding this comment.
We could almost infer this parameter from the func callable: if requesting _transform then you expect it to be fitted=True, if _fit in the name then fitted=False. I don't know if we having the keyword makes more explicit in some ways?
There was a problem hiding this comment.
fair enough, remember to leave this comment in the upcoming refactoring PR please 😁
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
| fitted=False, column_as_strings=False, replace_strings=True | ||
| ): | ||
| if step is None: | ||
| continue |
There was a problem hiding this comment.
It might be safer to have a test for this uncovered line.
@glemaitre doesn't seem like we're checking for a step being |
|
Fine with me if the tests are passing. Activating the auto-merge here. |
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
Towards #22893
This adds metadata routing to
ColumnTransformer.It also adds some docstrings to private methods and does a a bit of refactoring / clean up.
Fixes #19465
Fixes #24490