MAINT use composition in TableVectorizer #675
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glemaitre
commented
Jul 21, 2023
skrub/_table_vectorizer.py
Outdated
Comment on lines
839
to
850
| @property | ||
| def named_transformers_(self): | ||
| return self._column_transformer.named_transformers_ | ||
|
|
||
| @property | ||
| def sparse_output_(self): | ||
| return self._column_transformer.sparse_output_ | ||
|
|
||
| @property | ||
| def output_indices_(self): | ||
| return self._column_transformer.output_indices_ | ||
|
|
There was a problem hiding this comment.
We need tests to check those attributes.
|
I think you need to merge with main for the tests to run :) |
…ve_table_vectorizer # Conflicts: # skrub/_table_vectorizer.py # skrub/tests/test_table_vectorizer.py
Comment on lines
+711
to
+723
| # TODO: _check_feature_names raises a warning when fitting on dataframe | ||
| # but transforming on a numpy array. | ||
| # In practice, this looks error-prone and we need to discuss | ||
| # whether to raise an error instead. | ||
| # | ||
| # Note that when fitting on a dataframe and transforming on | ||
| # the same dataframe with different column names, | ||
| # _check_feature_names will raise an error. | ||
| self._check_feature_names(X, reset=reset) | ||
| feature_names = _get_feature_names(X) | ||
| feature_names_in = getattr(self, "feature_names_in_", None) | ||
| if feature_names is None and feature_names_in is not None: | ||
| X.columns = feature_names_in |
There was a problem hiding this comment.
Following our IRL discussion, @glemaitre
There was a problem hiding this comment.
@LeoGrin and @glemaitre this should fix #709! This is very similar to what is done in ColumnTransformer
jovan-stojanovic
approved these changes
Sep 6, 2023
|
Merging #592 makes this design more complex. I will probably restart from scratch since we need to handle the split/merge of the transformer that are parallelized. |
|
Should we close this PR @glemaitre? |
|
It will be automatically close when #761 will be merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #660
This is a POC for #660 that uses internally a
ColumnTransformerinstead of inheriting from it.