ENH make_sparse_spd_matrix use sparse memory layout
#27438
Conversation
There was a problem hiding this comment.
Thank you very much @Charlie-XIAO for the quick PR!
Here is a first pass of feedback:
|
Thanks for the review @ogrisel! I'm somehow busy rn but will try resolving them ASAP. |
|
@ogrisel Conversations resolved. As for backward compatibility, I added a new keyword |
|
I think we should use a sparse array instead of a sparse matrix. It means that to use this function with the non default sparse_format param, the user would need scipy 1.8 or later but i think this is fine for a new feature. |
Actually, looking at the code again, I think this PR is good the way it is. Let's wait and see at which speed scipy will deprecate scipy sparse matrices in favor of scipy sparse arrays. In particular, I expect that at some point top level functions used in this PR such as |
make_sparse_spd_matrix use sparse memory layoutmake_sparse_spd_matrix use sparse memory layout
There was a problem hiding this comment.
This looks good, thanks @Charlie-XIAO. I just have one question.
Reference Issues/PRs
Issue #27433.
What does this implement/fix? Explain your changes.
This PR implements
make_sparse_spd_matrixto use sparse memory layout from the start.Any other comments?
It seems that tests are missing for
make_sparse_spd_matrix. I made a test for the following:norm_diag=True, leading diagonal elements are 1;However, I found it hard to check the parameters
alpha,smallest_coef, andlargest_coef, because these are imposed onAbut the output isA.T * A. Due to the permutation (which I'm not sure why it is needed),Ais not necessarily upper/lower triangular so Cholesky decomposition does not giveAfrom the result. If the permutation is removed, it might be possible to test these parameters.