Exposing transformations applied by Cleaner and TableVectorizer - #2122
Exposing transformations applied by Cleaner and TableVectorizer#2122emassoulie wants to merge 21 commits into
Cleaner and TableVectorizer#2122Conversation
|
I've got something going with the transformation lists for both transformers! I feel the TableVectorizer transformations could be equally relevant to expose by raw transformer type ( |
…leaner-and-tablevectorizer-transformations
…leaner-and-tablevectorizer-transformations
…leaner-and-tablevectorizer-transformations
…leaner-and-tablevectorizer-transformations
…r-transformations
rcap107
left a comment
There was a problem hiding this comment.
Thank you very much for the PR @emassoulie ! I did a review of the current material.
In general, I think we're on the right path. I think the main thing to decide is the actual format of the returned string. I think there is too much whitespace, so we may want to investigate how other symbols may make it easier to parse the output.
Other than that, good job!
| check_is_fitted(self, "all_outputs_") | ||
| return np.asarray(self.all_outputs_) | ||
|
|
||
| def list_transformations(self): |
There was a problem hiding this comment.
This is a public method so it should have a docstring
…leaner-and-tablevectorizer-transformations
rcap107
left a comment
There was a problem hiding this comment.
Thanks a lot for improving the PR!
I think we're getting close to the end, though I think the code can still be improved and simplified in places.
I opened a PR targeting this branch in emassoulie#2 to propose some changes in that direction
Could you also add a mention of list_transformations in the docstring of TableVectorizer and Cleaner? Without printing the entire thing, only the first few lines followed by ... to elide the rest
| """Returns a string reporting the transformations applied by the table | ||
| vectorizer, and the columns they are each applied to. This covers every | ||
| preprocessing step, each of the `numeric`, `datetime`, `low cardinality` | ||
| and `high cardinality` transformations and any specific transformer. |
There was a problem hiding this comment.
numpy formatting requires a single line
| """Returns a string reporting the transformations applied by the table | |
| vectorizer, and the columns they are each applied to. This covers every | |
| preprocessing step, each of the `numeric`, `datetime`, `low cardinality` | |
| and `high cardinality` transformations and any specific transformer. | |
| """Returns a string reporting the transformations applied by the TableVectorizer \ | |
| and the columns they are each applied to. | |
| This covers every preprocessing step, each of the `numeric`, `datetime`, `low cardinality` | |
| and `high cardinality` transformations and any specific transformer. |
|
|
||
|
|
||
| def _limit_cols(col_names, max_cols=10): | ||
| list_cols = col_names[:max_cols] |
There was a problem hiding this comment.
I realize I suggested this version, but thinking about it again it could be simplified further to
list_cols = col_names[:max_cols] + ["..."] if len(col_names) > max_cols else col_namesThere was a problem hiding this comment.
though it's not a big problem since this is wrapped into a function anyway
| """ | ||
| For each column type (numeric, datetime etc.), there is a | ||
| dedicated transformer in the TableVectorizer that must be | ||
| displayed (for instance, self.numeric = Passthrough()). | ||
| The corresponding attribute is therefore fetched | ||
| and its class name printed. | ||
| """ |
There was a problem hiding this comment.
I'd rather have the regular comments with # even though it's a multi-line comment. It's how we do it in the rest of the codebase, and this is being rendered as a string so at a glance I was wondering if it would be printed
| check_is_fitted(self, "all_outputs_") | ||
| return np.asarray(self.all_outputs_) | ||
|
|
||
| def list_transformations(self, max_cols=10): |
| ) | ||
|
|
||
| t_post = self._postprocessors[0] | ||
| postprocessing_transformations += ( |
There was a problem hiding this comment.
maybe we can remove this section on postprocessing, since it's done to all columns anyway
it should be mentioned clearly in the docstring (I realize now it's not)
|
|
||
|
|
||
| def test_list_transformations(df_module): | ||
| def list_category(line_name, key, column_type="", with_specific=True, max_cols=3): |
There was a problem hiding this comment.
please add a comment here to explain what the function is doing
|
|
||
| return header + full_list | ||
|
|
||
| passthrough_line = [ |
There was a problem hiding this comment.
please add a small comment explaining what the test dataset looks like
| All float columns""" | ||
| assert vectorizer_output == expected_vectorizer_output | ||
|
|
||
| vectorizer = Cleaner(drop_if_constant=True) |
There was a problem hiding this comment.
the cleaner should be moved to a separate test, or the test should be parametrized to have both the cleaner and the tablevectorizer
in this case it may be simpler to have two separate tests, though that means repeating a lot of the code
either way, list_category should be moved outside of the test
There was a problem hiding this comment.
the reason I prefer having two separate tests (or a parametrized test) is that I was debugging this, and the test was failing on the Cleaner part
I did not notice that was the case because the diff was very long, so I was looking for the failure in the part about the TableVectorizer when it was in the Cleaner
| + list_category("DropUninformative", "uninformative", with_specific=False) | ||
| + list_category("Datetime", "datetime", with_specific=False) | ||
| ) | ||
| """ |
There was a problem hiding this comment.
same comment about using # rather than triple quotes
| Datetime (1 columns): | ||
| - datetime | ||
| """ | ||
| assert cleaner_output == expected_cleaner_output |
There was a problem hiding this comment.
same comment about testing line by line


Closes #1265
Adds a method to the
CleanerandTableVectorizertransformers enabling them to give a human-readable list of the transformations (DropUninformative, ToFloat, ToDatetime) they are set to apply once fitted.