-
Notifications
You must be signed in to change notification settings - Fork 883
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
Fix to_encode option in encode_features #1123
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1123 +/- ##
==========================================
- Coverage 98.37% 98.36% -0.02%
==========================================
Files 126 126
Lines 13466 13477 +11
==========================================
+ Hits 13247 13256 +9
- Misses 219 221 +2
Continue to review full report at Codecov.
|
try: | ||
new_X[c] = pd.to_numeric(new_X[c], errors='raise') | ||
except (TypeError, ValueError): | ||
pass |
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.
If we can't figure out why this is here/if it's still necessary, maybe we should add a warning so that we can at least tell when it does end up throwing this error instead of quietly passing
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.
If we do add a warning, I think it'd be good to also test the warning to increase code coverage.
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.
Our current tests don't cover this except block - we would need to find an example where this could occur
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.
In the interest of getting this fix into today's release, I'm open to doing a separate issue / PR cycle on the usefulness of this block / how we want to handle the exceptions.
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 👍
Fixes #1115
encode_features
has an optionto_encode
that allows users to specify a list of features they want encoded.encode_features
was only partially using the info provided fromto_encode
-- it was only creating encoded features from the features specified byto_encode
, but it was converting all features to a numeric pandas dtype instead of just the encoded ones.