FIX Raise explicit exception for input with complex dtype - #7729
Conversation
Increases the number of common checks we pass and prevent silent conversion to real.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded runtime checks to reject complex-valued inputs in CumlArray.from_input, added a unit test asserting KMeans raises ValueError on complex data for numpy and cupy, removed many per-estimator complex-data xfail expectations, and pruned corresponding upstream xfail entries. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Needs #7762 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuml/cuml/internals/array.py (1)
999-1013:⚠️ Potential issue | 🟠 MajorCumlArray fast-path bypasses the new complex-dtype check.
If a
CumlArrayholding complex data is passed tofrom_inputand all fast-path conditions are met (lines 999–1009), it is returned (or deep-copied) without ever hitting the complex-dtype guard at line 1060. This leaves a gap where complex data can silently flow through.Consider adding the complex check before the fast-path return, e.g.:
Proposed fix
if ( isinstance(X, CumlArray) and (not convert_to_mem_type or convert_to_mem_type == X.mem_type) and (not convert_to_dtype or convert_to_dtype == X.dtype) and (not force_contiguous or X.is_contiguous) and (order in ("K", None) or X.order == order) and not check_dtype and not check_mem_type and not check_cols and not check_rows ): + if np.issubdtype(X.dtype, np.complexfloating): + raise ValueError(f"Complex data not supported\n{X}") if deepcopy: return copy.deepcopy(X) else: return XBased on learnings: "Silent data corruption from type coercion, incorrect handling of cuDF vs pandas vs NumPy inputs, or missing validation causing crashes on invalid input must be addressed."
|
@coderabbitai I think via the public API you can't pass complex data to the fast path. So maybe this is out of scope. But the fix is easy and made me think about adding a test so implemented it |
|
Tip For best results, initiate chat on the files or code changes.
That said, I think keeping this fix is valuable for a few reasons:
Given that it's already implemented, tested, and working (enabling removal of many xfail entries), I'd lean toward keeping it. But if you feel strongly that it's truly unreachable and adds unnecessary complexity, removing it is also reasonable. What are your thoughts? 🧠 Learnings used |
|
/merge |
Increases the number of common checks we pass and prevents silent conversion to real. So far estimators in cuml would silently drop the imaginary part and fit on just the real part. Probably not what users were expecting. Authors: - Tim Head (https://github.com/betatim) - Simon Adorf (https://github.com/csadorf) Approvers: - Jim Crist-Harif (https://github.com/jcrist) URL: NVIDIA#7729
5 participants
Increases the number of common checks we pass and prevents silent conversion to real. So far estimators in cuml would silently drop the imaginary part and fit on just the real part. Probably not what users were expecting.