TST Extend tests for scipy.sparse.*array in sklearn/covariance/tests/test_graphical_lasso.py
#27494
Conversation
|
You should only add CSR_CONTAINERS parameterisation where scipy.sparse.csr_matrix is already used |
|
But its not being used anywhere directly in the file. Other than that it appears quite beneficial to switch all the dense matrices here to sparse matrices. Please let me know what are your thoughts. |
|
There were files where sparse matrices were not used at all, as I recall you should simply leave them as they are. But in your case you can parameterise function make_sparse_spd_matrix, because after recent PR it can return sparse matrices. Honestly, But I am not sure if it's necessary, you better ask maintainers, it's up to them. If it's not there is no parameterisation to be added in this file. Should make_sparse_spd_matrix be parameterised to return sparse and dense matrices in this file? @glemaitre |
|
I'll be patiently waiting for a response. Till then I'll focus on another issue and leave this PR open. |
We merge a change not long time ago that allow to return a sparse matrix. But here, all tests are not interesting about the sparse format of the input but rather the sparsity of the array itself. Since that there is no call to I am closing this PR and note this is file as done in the issue tracker. Thanks @KartikeyBartwal for investigating. |
|
frankly, I was expecting this to be my first PR to get merged. Had been working on this file for over 2 weeks. No problem ! Let's work on another assigned file 🙌 |
Reference Issues/PRs
Towards #27090
Any other comments?
Loving the contribution tasks. This improvement in just one pull request is quite satisfying :)
Interested in further contributions 🙌