ENH: Return rank 0 for empty matrices in matrix_rank#30422
ENH: Return rank 0 for empty matrices in matrix_rank#30422seberg merged 11 commits intonumpy:mainnumpy/numpy:mainfrom false200:fix-matrix-rank-emptyfalse200/numpy:fix-matrix-rank-emptyCopy head branch name to clipboard
Conversation
Fix trailing whitespace causing lint failure and simplify the empty-matrix rank assertion.
|
|
||
| # Handle empty matrices - rank is 0 for matrices with 0 rows or 0 columns | ||
| if A.size == 0: | ||
| return 0 |
There was a problem hiding this comment.
I wonder if we should just add initial=0 which I assume also works. The tests you added are missing the case of np.zeros((0, 5, 5)) where this return is clearly incorrect.
EDIT: Actually, while we are here, can you check if we don't return np.intp(0) on the last return? We may want to adjust the return type for the other early return as well.
There was a problem hiding this comment.
Hi @seberg, thanks for the feedback!
Added initial=0 to the S.max(... ) call as suggested, and I also added the missing np.zeros((0, 5, 5)) test.
I ran the linalg test suite locally and everything looks good.
All relevant tests passed, only the usual SKIP/XFAIL cases showed up.
Let me know if you'd like any additional tweaks!
There was a problem hiding this comment.
Hi @seberg , just a quick ping , all checks have passed now.
Would you mind taking a look for merge? Thanks!
Added a newline at end of file to satisfy linter.
Ensures exactly one blank line at the end of numpy/linalg/tests/test_linalg.py
seberg
left a comment
There was a problem hiding this comment.
Code looks good now (I might punt on backporting, but should be OK too).
The tests could be nicer with parametrization, but it is OK.
I am not quite 100% on whether zero is always expected, so wondering if you can argue that or know some prior art?
|
@seberg Thanks for the careful review and the questions
So this change doesn’t introduce a new definition so much as make the existing mathematical convention explicit and handle it robustly in code. |
|
Cool, thanks for confirming, agreed seems rather clear convention, so let's put this in, thanks. Should be safe for backporting, but I would probably just consider it an enhancement. |
|
@seberg Thanks for clarifying! |
This PR fixes a bug in np.linalg.matrix_rank where empty matrices (previously with 0 rows or 0 columns) would raise a ValueError due to attempting a reduction operation on a zero-size array.
ENH: Return rank 0 for empty matrices in matrix_rank (#30422)
This PR fixes a bug in
np.linalg.matrix_rankwhere empty matrices(previously with 0 rows or 0 columns) would raise a ValueError due to
attempting a reduction operation on a zero-size array.
Changes include:
matrix_rank.Files modified:
numpy/linalg/_linalg.pynumpy/linalg/tests/test_linalg.pyCloses #30421