FIX Add input array check to randomized_range_finder - #30819
Conversation
clane9
changed the title
randomized_range_finderrandomized_range_finder
| A = check_array(A, accept_sparse=True) | ||
| xp, is_array_api_compliant = get_namespace(A) |
There was a problem hiding this comment.
This is going to basically cancel the array API work I think.
I wonder if explicitly checking for complex values is the only way.
cc @OmarManzoor maybe
There was a problem hiding this comment.
Ah, you're right. Thanks for the catch. What if we just call check_array but not reassign A?
There was a problem hiding this comment.
I think if we don't assign the new array to A, it should work like before while handling the exceptions we need to catch. By the way I think check_array supports the array api itself.
There was a problem hiding this comment.
Ok, made the change to not reassign A after check_array.
I also tried check_array on a torch Tensor. The check runs and passes but it does convert to a numpy ndarray.
There was a problem hiding this comment.
check_array is array-api compliant so it can be used as usual.
There was a problem hiding this comment.
@jeremiedbb, I see, when I tried out check_array on a torch Tensor previously, I didn't have the array api enabled.
|
|
| @@ -0,0 +1,2 @@ | |||
| - :func:`sklearn.utils.extmath.randomized_svd` now support Array API compatible inputs. | |||
| By :user:`Connor Lane <clane9>`. No newline at end of file | |||
There was a problem hiding this comment.
Maybe @jeremiedbb you could add yourself as a co-author of this contribution ;)
|
Thanks for the help with this PR @jeremiedbb and @ogrisel! |
|
@jeremiedbb I let you credit your work in the changelog and merge if you wish. |
|
Thanks @clane9 and sorry for the late feedback on this PR. |
lesteve
left a comment
lesteve
left a comment
There was a problem hiding this comment.
Do we want to add a test to check that randomized_svd and randomized_range_finder raise a meaninful error on invalid data (e.g. complex data like in the original issue) or is it overkill?
|
The thing is that we don't have checks for complex input for any other function so I would not add it here and trust the |
…inder` (scikit-learn#30819) Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
6 participants
Reference Issues/PRs
Fixes #30736. See also #30737.
What does this implement/fix? Explain your changes.
Adds a check of the input array to
randomized_range_finderso that complex valued inputs are rejected. As a result, complex inputs are also rejected fromrandomized_svd, which callsrandomized_range_finder.