Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
The other way would be to just reject object arrays and let the user handle conversion to a an inexact type. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #976 +/- ##
=======================================
Coverage 98.95% 98.95%
=======================================
Files 137 137
Lines 30099 30124 +25
Branches 2337 2343 +6
=======================================
+ Hits 29785 29810 +25
Misses 256 256
Partials 58 58
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Rejecting object arrays would simplify this path. The existing dtype-policy tests allow conversion of object inputs, so this patch preserves that behavior and fixes the loss of imaginary components within it. Would you prefer to retain those conversions for this fix, or reject object inputs here? The validation cases can be adjusted to the chosen input policy. |
bashtage
left a comment
bashtage
left a comment
There was a problem hiding this comment.
I'm OK with this direction. I think the performance of the complex check can be improved using a 2-step approach, try astype complex128, if fail, it is not numeric, then if pass, check if imag are all 0. If all 0, then downcast to float64. Can avoid python loops with this method.
| f"({type(x).__name__})" | ||
| ) | ||
| try: | ||
| if any(isinstance(item, (complex, np.complexfloating)) for item in arr.flat): |
There was a problem hiding this comment.
How about this. Rather than a loop, if allowed, we try to cast astype(np.complex128). If this fails, then we know it is not numeric. If it succeeds, we can then do a single pass to check if all imag values are 0, and if they are, recast unsafe to float64. This would use more numpy and less python looping.
| """ | ||
| try: | ||
| dtypes = list(x.dtypes) if isinstance(x, DataFrame) else [x.dtype] | ||
| if target is np.float64 and any(dtype.kind == "O" for dtype in dtypes): |
There was a problem hiding this comment.
Should pull this up into a single helper function along with the other check of object could be complex. Then there is one definitive method, whether it is optimized or not.
as_float64 first tries astype(float64) on object arrays. Python complex values raise TypeError and reach the complex conversion path, but NumPy complex scalars can instead cast to real with a ComplexWarning. An object array containing np.complex128(1+2j) therefore becomes [1.] even with allow_complex=True; with allow_complex=False it also passes instead of raising.
Detect complex scalar elements before casting, in both the ndarray and pandas conversion paths. Convert permitted complex object data to complex128 and reject it through the existing TypeError path when complex data are not allowed.
Add both cases to the dtype policy table and assert numerical values as well as output dtype for successful object conversions. Both NumPy-complex cases fail before the fix. The complete dtype policy module passes after the fix (485 tests), including 24 cases spanning four helpers and ndarray, Series and DataFrame inputs; Ruff lint and formatting pass for both changed files. This follows the documented complex-data policy in doc/source/contributing.rst.