-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
ENH: Add use_nullable_dtypes to to_numeric #50505
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
Conversation
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.
- Could you add tests that exercises the
errors
keyword argument?
Thx, this found a couple of bugs |
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.
Can you add a test where use_nullable_dtype=True
is specified but the input already is a nullable array?
if mask is None: | ||
mask = new_mask |
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 mask is None: | |
mask = new_mask | |
if mask is None: | |
mask = new_mask | |
else: | |
mask = mask.copy() |
and then remove the .copy()
below when constructing the array (right now the mask is always copied, but assuming this newly generated mask doesn't need that)
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.
DOne
# Conflicts: # doc/source/whatsnew/v2.0.0.rst
Added a test and found a problem for BooleanArray |
Merging for now, happy to address additional comments in a follow up |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.I think to_numeric should be able to return nullable dtypes as well, similar to our I/O functions