Skip to content

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented Feb 17, 2024

Fixes #25840 - the problem was that for size_t, 0 - 1 is a large number. Since a fortran int is passed in, a ssize_t makes more sense anyway.

@mhvk mhvk added this to the 2.0.0 release milestone Feb 17, 2024
@mhvk mhvk force-pushed the linalg-cholesky-upper-bug-for-empty-array branch from a126bd1 to 13c3c42 Compare February 17, 2024 22:51
@mhvk
Copy link
Contributor Author

mhvk commented Feb 17, 2024

With just ssize_t somehow I get errors on windows (older python?). See https://github.com/numpy/numpy/actions/runs/7944728697
Try changing to Py_ssize_t.

@mhvk
Copy link
Contributor Author

mhvk commented Feb 17, 2024

Or maybe it should just be fortran_int???

@charris
Copy link
Member

charris commented Feb 18, 2024

Looks like it is called with fortran_int, but the size_t goes back before c++, but was still called with a fortran_int. That seems a dangerous combination, as C casts signed to unsigned without warnings. The affected routines are only called by cholesky, so changing the argument type should work. Special casing zero might be a good idea, a zero check should be very cheap.

The original code goes back to 2012. I was wondering, as we seldom use size_t.

@mhvk
Copy link
Contributor Author

mhvk commented Feb 18, 2024

@charris - do you think it makes sense to just use fortran_int in the loop? It is not like this is going to be anything other than int32 or int64 anyway.

EDIT: though tests pass now, at least.

@charris
Copy link
Member

charris commented Feb 18, 2024

Lets go with fortran_int. That is what is passed in, and we are dealing with a matrix returned by an LAPACK call.
If inconsistencies turn up, they can be dealt with at a higher level.

The functions triu_matrix and tril_matrix could use a rename, as what they do is zero the unused part of the matrix returned by LAPACK, maybe something like clean_fortran_triu, etc., with a documentation comment. But that might be out of scope here.

If there was no chance of other uses in the future, I would just pass in params.

@mhvk mhvk force-pushed the linalg-cholesky-upper-bug-for-empty-array branch from 13c3c42 to 18d1891 Compare February 18, 2024 14:11
@mhvk
Copy link
Contributor Author

mhvk commented Feb 18, 2024

@charris - Sounds good, I went with all your suggestions - the zeroing is done for Cholesky only, so it makes sense to keep it close (especially as it is meant to be inlined).

@charris charris merged commit 1196d03 into numpy:main Feb 18, 2024
@charris
Copy link
Member

charris commented Feb 18, 2024

Good name choice! Thanks Marten.

The failing test is unrelated, not sure what is going on there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: cholesky(empty((0, 0)), upper=True) hangs

2 participants