TST Extend tests for scipy.sparse/*array in sklearn/manifold/tests/test_spectral_embedding

[Charlie-XIAO]

Towards #27090.

[github-actions]

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: bb9c6b5. Link to the linter CI: here}

[glemaitre]

Otherwise LGTM.

[glemaitre]

Otherwise LGTM.

[Charlie-XIAO]

Otherwise LGTM.

I think there are still some failing tests, and I'm looking into it. Previously I did not have pyamg installed so I overlooked those errors.

[glemaitre]

I don't see the traceback but I assume that somewhere we have if issparse(X) and the process in this branch was returning a np.matrix. In the previous PR, that I reviewed, I just check that you can replace if isspmatrix(X) because in the case of the sparse arrays, the operation will return a NumPy array that is what is expected as well for the dense case.

I have to go back to some previous PR to check if I omit such changes.

[Charlie-XIAO]

This case seems to be a bit different, I got:

>       laplacian, dd = csgraph_laplacian(
            adjacency, normed=norm_laplacian, return_diag=True
        )

sklearn\manifold\_spectral_embedding.py:279:

>       lap, d = create_lap(
            csgraph,
            normed=normed,
            axis=degree_axis,
            copy=copy,
            form=form,
            dtype=dtype,
            symmetrized=symmetrized,
        )

D:\Downloads\mambaforge\envs\sklearn-env\lib\site-packages\scipy\sparse\csgraph\_laplacian.py:354:

>       w = m.sum(axis=axis).getA1() - m.diagonal()
E       AttributeError: 'numpy.ndarray' object has no attribute 'getA1'

D:\Downloads\mambaforge\envs\sklearn-env\lib\site-packages\scipy\sparse\csgraph\_laplacian.py:459: AttributeError

This seems to imply that scipy.sparse.csgraph.laplacian does not accept sparse arrays as input. I can indeed do something like

if sparse.issparse(adjacency) and not sparse.isspmatrix(adjacency):
    adjacency = adjacency.toarray()

But this does not seem to be desired. Somehow I need a method to convert xxx_array to xxx_matrix but I read through scipy sparse array documentation and did not seem to find one? Or is there a better workaround? @glemaitre

[glemaitre]

Actually, there is a bug with isspmatrix for SciPy < 1.11 where it returns True even for sparse arrays. So we cannot do what I propose. I will check this PR accordingdly.

[glemaitre]

But this does not seem to be desired. Somehow I need a method to convert xxx_array to xxx_matrix but I read through scipy sparse array documentation and did not seem to find one? Or is there a better workaround?

Indeed, we can convert it by calling the *_matrix() constructor on the array. We just need to be sure which type of sparse format we have. We can make it a temporary fix.

I will open an issue/PR in SciPy to fix it.

[glemaitre]

OK. So the bug has been solved in scipy/scipy#19156
One potential thing that we could do here is to vendor the file for sometimes as we did in the past.

Would you mind to copy paste the _laplacian.py file from scipy main branch into sklearn/externals folder. Then, we need to edit sklearn.utils.fixes where we will import laplacian from SciPy only if the version is > 1.11, otherwise we import the version from externals. Then, we need to change the import in the _spectral_embedding.py to import from sklearn.utils.fixes import laplacian instead from the SciPy import.

[Charlie-XIAO]

Sure, I don't have time at this moment but will revisit in a day.

[Charlie-XIAO]

@glemaitre Do we need to write some comment at the beginning of the _laplacian.py file to mention that this is vendored from scipy?

[glemaitre]

Do we need to write some comment at the beginning of the _laplacian.py file to mention that this is vendored from scipy?

I will do that and add an additional entry in the changelog acknowledging the non-support for 64-bits indices in SpectralEmbedding with the AMG solver.

[glemaitre]

I checked that merging #27372 and solve the issue in this PR.

[lesteve]

Looking at the failure, it seems like this has been fixed in scipy 1.11.2 scipy/scipy#18644. It looks like the nomkl environment can only get scipy 1.11.1 because it is using the defaults conda channel and scipy 1.11.2 is not available yet on the defaults conda channel ...

[glemaitre]

@lesteve But it means that we don't support SciPy from 1.8 to 1.11.1.

[lesteve]

Maybe we can find a way to convey that sparse array support is experimental for now?

[ogrisel]

Thanks @jeromedockes for your analysis, I opened scipy/scipy#19246 upstream.

[ogrisel]

Maybe we can find a way to convey that sparse array support is experimental for now?

I would be fine stating in the changelog that the support of scipy sparse arrays in scikit-learn estimators requires the latest release of scipy, or even the future 1.12.0 (even if scipy sparse arrays were introduced in 1.8).

[jeromedockes]

Thanks @jeromedockes for your analysis, I opened scipy/scipy#19246 upstream.

oops I think @glemaitre had already opened one

[glemaitre]

OK so we merged #27372, we should be able to go forward with this PR now. @Charlie-XIAO would you mind resolving the conflicts?

[Charlie-XIAO]

OK so we merged #27372, we should be able to go forward with this PR now. @Charlie-XIAO would you mind resolving the conflicts?

Sure, having followed the progress of that issue for long~

[Charlie-XIAO]

Strange that codecov is complaining about fixes.py, though I've already added # type: ignore # noqa. Should I do something else to resolve this @glemaitre?

[Charlie-XIAO]

Done, I think things are looking good so far. @glemaitre

[glemaitre]

LGTM. We will need another reviewer on this.

[jjerphan]

Thanks, @Charlie-XIAO!

A few comments and suggestions regarding mentioning and integrating the recent support of int64 in SciPy graph traversal routine; otherwise LGTM.

[jjerphan]

Can you indicate the following in this header

• the license of this code
• the reason for vendoring the code
• potential modifications

?

[Charlie-XIAO]

I have added the license and reason for vendoring the code. There is no modification, and I have provided the PR link in scipy that solved the issue.

[Charlie-XIAO]

@jjerphan Sorry for my delayed response. I have addressed your suggestions, so would you mind taking another look?

[jjerphan]

LGTM!