-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
CLN: clean-up sanitize_array series construction #26979
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
CLN: clean-up sanitize_array series construction #26979
Conversation
Codecov Report
@@ Coverage Diff @@
## master #26979 +/- ##
==========================================
- Coverage 91.98% 91.97% -0.01%
==========================================
Files 180 180
Lines 50760 50754 -6
==========================================
- Hits 46690 46682 -8
- Misses 4070 4072 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26979 +/- ##
==========================================
+ Coverage 91.84% 91.99% +0.14%
==========================================
Files 180 180
Lines 50734 50768 +34
==========================================
+ Hits 46599 46705 +106
+ Misses 4135 4063 -72
Continue to review full report at Codecov.
|
subarr = data.copy() | ||
else: | ||
subarr = _try_cast(data, True, dtype, copy, raise_cast_failure) | ||
elif isinstance(data, Index): |
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 was already checked above that data
is an ndarray, so can never be an index
|
||
elif isinstance(data, ExtensionArray): | ||
if isinstance(data, ABCPandasArray): |
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.
The extract_array(data, extract_numpy=True)
above already did this
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. small comments.
subarr = np.array(data, copy=False) | ||
|
||
if (dtype is not None | ||
and is_float_dtype(data.dtype) and is_integer_dtype(dtype)): |
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.
future cleanup is to remove this and condition (as I think _try_cast already handles this)
if dtype is not None: | ||
subarr = data.astype(dtype) | ||
|
||
subarr = subarr.astype(dtype) |
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.
add copy=False here (not sure if it matters but can't hurt)
@@ -677,10 +657,10 @@ def sanitize_array(data, index, dtype=None, copy=False, | |||
return subarr | |||
|
|||
|
|||
def _try_cast(arr, take_fast_path, dtype, copy, raise_cast_failure): | |||
def _try_cast(arr, dtype, copy, raise_cast_failure): | |||
|
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 try to doc-string / type this?
@@ -677,10 +657,22 @@ def sanitize_array(data, index, dtype=None, copy=False, | |||
return subarr | |||
|
|||
|
|||
def _try_cast(arr, take_fast_path, dtype, copy, raise_cast_failure): | |||
|
|||
def _try_cast(arr, dtype, copy, raise_cast_failure): |
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 types here?
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.
Nice removing the fast path arg!
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.
@jorisvandenbossche can you add types here?
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, if you can add types to _try_cast would be great. merge on green.
|
||
Parameters | ||
---------- | ||
arr : array-like |
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.
Any restriction on ndim? Some of these functions are only called via the dataframe constructor.
@jbrockmendel ok with this? |
yep |
thanks @jorisvandenbossche |
Some possible clean-up that I encountered when doing #26848