ENH: Allow use of svd on empty arrays#11424
Conversation
There was a problem hiding this comment.
The changes are probably good, I guess there are practically tests run now when deleting those lines, but... IIRC the svd should probably have a specific form at least when full_matrices is set to True, and I doubt that this is already being tested?
| delinearize_@REALTYPE@_matrix(args[1], params.S, &s_out); | ||
| } else { | ||
| if ('A' == params.JOBZ && min_m_n == 0) { | ||
| /* Lapack has betrayed us and left these uninitialized, |
There was a problem hiding this comment.
NIT: comment should start on the next line.
|
Heh, that was quicker then possible ;), LGTM with the new test now. |
|
Hmm, actually, I am not 100% sure right now if this goes into Lapack, I am a bit worried that things might not work for annoying lapack implementations. And the thing is on a quick look over I am not sure it does not sometimes go into lapack. @eric-wieser are you pretty sure it is either not going in or lapack standard should error out cleanly? |
|
|
|
This does go into lapack, but it passes parameters that conform. Typically our mistakes in this area have been setting I'd probably say put it in - there's no way for lapack to produce an invalid result here, since in the 0-size case all of the outputs are overwritten after the call anyway. If it returns an error code, then we can handle that if/when we get a report of it. And if it clobbers arbitrary memory... Then I think the authors of the lapack implementation need strong words. |
|
OK, if you are certain enough that this is safe. I don't really know these lapack details well, vaguely remember that max(M, 1) thing. It also seems a bit like we are already doing this without complains in other functions, so seems fine. |
|
OK, tests succeed, so will put this in. Thanks Eric! |
part of #8654
Revived from over a year ago
cc @convexset