TST Extend tests for `scipy.sparse.*array` in `sklearn/svm/tests/test_sparse` by Charlie-XIAO · Pull Request #27723 · scikit-learn/scikit-learn

Charlie-XIAO · 2023-11-04T16:03:01Z

Towards #27090, continuation of #27511.

…arse

Co-authored-by: Mohit Joshi <46566524+work-mohit@users.noreply.github.com>

github-actions · 2023-11-04T16:04:15Z

✔️ Linting Passed

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

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

Charlie-XIAO · 2023-11-11T08:50:26Z

@jjerphan I have fixed the bug met by @work-mohit, would you like to take a look?

jjerphan

LGTM modulo a few changes and a changelog entry.

sklearn/svm/tests/test_sparse.py

…Julien)

jjerphan · 2023-11-12T09:31:10Z

One last thing: could you add an entry in this section for 1.4's changelog? :)

scikit-learn/doc/whats_new/v1.4.rst

Lines 132 to 182 in 8db3aac

    
           Support for SciPy sparse arrays 
        
           ------------------------------- 
        
           Several estimators are now supporting SciPy sparse arrays. The following functions 
        
           and classes are impacted: 
        
           **Functions:** 
        
           - :func:`cluster.compute_optics_graph` in :pr:`27104` by 
        
             :user:`Maren Westermann <marenwestermann>` and in :pr:`27250` by :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :func:`cluster.kmeans_plusplus` in :pr:`27179` by :user:`Nurseit Kamchyev <Bncer>`; 
        
           - :func:`decomposition.non_negative_factorization` in :pr:`27100` by 
        
             :user:`Isaac Virshup <ivirshup>`; 
        
           - :func:`feature_selection.f_regression` in :pr:`27239` by 
        
             :user:`Yaroslav Korobko <Tialo>`; 
        
           - :func:`feature_selection.r_regression` in :pr:`27239` by 
        
             :user:`Yaroslav Korobko <Tialo>`; 
        
           - :func:`manifold.trustworthiness` in :pr:`27250` by :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :func:`manifold.spectral_embedding` in :pr:`27240` by :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :func:`metrics.pairwise_distances` in :pr:`27250` by :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :func:`metrics.pairwise_distances_chunked` in :pr:`27250` by 
        
             :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :func:`metrics.pairwise.pairwise_kernels` in :pr:`27250` by 
        
             :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :func:`sklearn.utils.multiclass.type_of_target` in :pr:`27274` by 
        
             :user:`Yao Xiao <Charlie-XIAO>`. 
        
           **Classes:** 
        
           - :class:`cluster.HDBSCAN` in :pr:`27250` by :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :class:`cluster.KMeans` in :pr:`27179` by :user:`Nurseit Kamchyev <Bncer>`; 
        
           - :class:`cluster.MiniBatchKMeans` in :pr:`27179` by :user:`Nurseit Kamchyev <Bncer>`; 
        
           - :class:`cluster.OPTICS` in :pr:`27104` by 
        
             :user:`Maren Westermann <marenwestermann>` and in :pr:`27250` by :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :class:`decomposition.NMF` in :pr:`27100` by :user:`Isaac Virshup <ivirshup>`; 
        
           - :class:`decomposition.MiniBatchNMF` in :pr:`27100` by 
        
             :user:`Isaac Virshup <ivirshup>`; 
        
           - :class:`feature_extraction.text.TfidfTransformer` in :pr:`27219` by 
        
             :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :class:`cluster.Isomap` in :pr:`27250` by :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :func:`manifold.SpectralEmbedding` in :pr:`27240` by :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :class:`manifold.TSNE` in :pr:`27250` by :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :class:`impute.SimpleImputer` in :pr:`27277` by :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :class:`impute.IterativeImputer` in :pr:`27277` by :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :class:`impute.KNNImputer` in :pr:`27277` by :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :class:`kernel_approximation.PolynomialCountSketch` in  :pr:`27301` by 
        
             :user:`Lohit SundaramahaLingam <lohitslohit>`; 
        
           - :class:`neural_network.BernoulliRBM` in :pr:`27252` by 
        
             :user:`Yao Xiao <Charlie-XIAO>`; 
        
           - :class:`preprocessing.PolynomialFeatures` in :pr:`27166` by 
        
             :user:`Mohit Joshi <work-mohit>`.

Charlie-XIAO · 2023-11-12T10:24:32Z

One last thing: could you add an entry in this section for 1.4's changelog? :)

I’m thinking that, I haven’t modified a single line in the source code, which means that sparse arrays are originally supported and not a new feature in this case. As far as I’m concerned, the functions and classes in that section of changelog did not support sparse array previously, either because they were using .dot for sparse matrix multiplication or some more complex reasons.

Please let me know if this is not the case :)

jjerphan

Then LGTM after a green CI.

For some estimators supporting sparse matrices, sparse arrays were supported implicitly.

I thought this section in the changelog was added so that we can report the support of sparse arrays explicitly for all the interfaces covered by the tests, even for the ones which have been supporting them implicitly.

Probably the changelog entry needs to be clarified?

Anyway, thank you, @Charlie-XIAO. :)

Charlie-XIAO · 2023-11-12T12:35:37Z

Oh wait a minute @jjerphan, I think there are failing CIs. Probably it is because changing assert_array_almost_equal to assert_allclose? I'm not sure about the difference between these two.

jjerphan · 2023-11-12T12:37:45Z

Feel free to revert this change, we can remove this old, deprecated interface as part of another PR.

Charlie-XIAO · 2023-11-12T12:40:52Z

Okay I see, thanks for the response! Though I may have to do that a little bit later.

jjerphan · 2023-11-12T14:07:46Z

Yes, no pressure. :)

It is worth enjoying life on weekends.

glemaitre

LGTM. Thanks @Charlie-XIAO

Charlie-XIAO and others added 3 commits November 4, 2023 23:54

TST Extend tests for scipy.sparse.*array in sklearn/svm/tests/test_sp…

aa189e4

…arse

Merge remote-tracking branch 'upstream/main' into tst-sp-svm-sparse

de5541e

credit to work-mohit

90f0052

Co-authored-by: Mohit Joshi <46566524+work-mohit@users.noreply.github.com>

github-actions bot added the module:svm label Nov 4, 2023

Charlie-XIAO mentioned this pull request Nov 4, 2023

TST Extend tests for scipy.sparse.*array in sklearn/svm/tests/test_sparse.py #27511

Closed

glemaitre added the No Changelog Needed label Nov 4, 2023

Charlie-XIAO added 2 commits November 11, 2023 14:38

Merge remote-tracking branch 'upstream/main' into tst-sp-svm-sparse

8fcbbda

fix int64 not accepted problem; use tocoo for simplicity

6304cd1

jjerphan reviewed Nov 11, 2023

View reviewed changes

sklearn/svm/tests/test_sparse.py Outdated Show resolved Hide resolved

sklearn/svm/tests/test_sparse.py Outdated Show resolved Hide resolved

sklearn/svm/tests/test_sparse.py Show resolved Hide resolved

jjerphan removed the No Changelog Needed label Nov 11, 2023

Charlie-XIAO added 2 commits November 11, 2023 23:04

replace assert_array_almost_equal with assert_allclose (suggested by …

ad169df

…Julien)

fix array_allclose unexpected keyword decimal

9629ba8

jjerphan approved these changes Nov 12, 2023

View reviewed changes

jjerphan mentioned this pull request Nov 12, 2023

TST Extend tests for scipy.sparse.*array #27090

Closed

jjerphan added the No Changelog Needed label Nov 12, 2023

Charlie-XIAO added 3 commits November 13, 2023 01:34

partially revert to assert_allclose for failing tests

e068379

Merge remote-tracking branch 'upstream/main' into tst-sp-svm-sparse

a291d76

revert more back to assert_array_almost equal b/c CI errors

e33f062

jjerphan added the Waiting for Second Reviewer First reviewer is done, need a second one! label Nov 13, 2023

glemaitre self-requested a review November 18, 2023 14:55

import from sklearn instead of numpy

c481b3d

glemaitre approved these changes Nov 18, 2023

View reviewed changes

glemaitre enabled auto-merge (squash) November 18, 2023 15:11

glemaitre merged commit 9e8f10e into scikit-learn:main Nov 18, 2023

Charlie-XIAO deleted the tst-sp-svm-sparse branch December 12, 2023 15:38

Uh oh!

Conversation

Charlie-XIAO commented Nov 4, 2023

Uh oh!

github-actions bot commented Nov 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

Charlie-XIAO commented Nov 11, 2023

Uh oh!

jjerphan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jjerphan commented Nov 12, 2023

Uh oh!

Charlie-XIAO commented Nov 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjerphan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Charlie-XIAO commented Nov 12, 2023

Uh oh!

jjerphan commented Nov 12, 2023

Uh oh!

Charlie-XIAO commented Nov 12, 2023

Uh oh!

jjerphan commented Nov 12, 2023

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Nov 4, 2023 •

edited

Loading

Charlie-XIAO commented Nov 12, 2023 •

edited

Loading

jjerphan left a comment •

edited

Loading