-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TextFeaturizer test coverage update #1122
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1122 +/- ##
==========================================
+ Coverage 99.91% 99.93% +0.01%
==========================================
Files 194 194
Lines 10957 10959 +2
==========================================
+ Hits 10948 10952 +4
+ Misses 9 7 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just wanted to better understand why the deleted code was dead code but otherwise looks good :D
@@ -58,11 +58,6 @@ def _make_entity_set(self, X, text_columns): | |||
es = self._ft.EntitySet() | |||
es.entity_from_dataframe(entity_id='X', dataframe=X_text, index='index', make_index=True, | |||
variable_types=all_text_variable_types) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this dead code because we do the filtering for text variable types before we call _make_entity_set
? :o
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's dead because we now explicitly set all text columns to the Text type when generating the entityset (line 60), so they're guaranteed to all be the correct type during the deleted check!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, makes sense to me 🚢
Fixes #1120