TST Extend tests for `scipy.sparse.*array` in `sklearn/utils/tests/test_param_validation.py` by Charlie-XIAO · Pull Request #27950 · scikit-learn/scikit-learn

Charlie-XIAO · 2023-12-12T15:34:37Z

Reference Issues/PRs

Towards #27090.

What does this implement/fix? Explain your changes.

Extend validation for sparse containers. This would require changing "sparse matrix" to "sparse container" when necessary. Previous discussion at #27317.

The following are the changes I have made (because tests are failing):

Details

sklearn.cluster
    cluster.dbscan
        - sklearn/cluster/tests/test_dbscan.py::test_dbscan_input_not_modified
        - sklearn/cluster/tests/test_dbscan.py::test_dbscan_input_not_modified_precomputed_sparse_nodiag
    cluster.compute_optics_graph
        - sklearn/cluster/tests/test_optics.py::test_correct_number_of_clusters (*)
        - sklearn/cluster/tests/test_optics.py::test_dbscan_optics_parity (*)
        - sklearn/cluster/tests/test_optics.py::test_min_cluster_size_invalid2 (*)
        - sklearn/cluster/tests/test_optics.py::test_precomputed_dists (*)
sklearn.neighbors
    neighbors.sort_graph_by_row_values
        - sklearn/cluster/tests/test_dbscan.py::test_dbscan_precomputed_metric_with_initial_rows_zero (*)
        - sklearn/neighbors/tests/test_neighbors.py::test_sort_graph_by_row_values
        - sklearn/neighbors/tests/test_neighbors.py::test_sort_graph_by_row_values_copy
        - sklearn/neighbors/tests/test_neighbors.py::test_sort_graph_by_row_values_warning
        - sklearn/neighbors/tests/test_neighbors.py::test_sort_graph_by_row_values_bad_sparse_format
        - sklearn/neighbors/tests/test_neighbors.py::test_precomputed_sparse_invalid
        - sklearn/neighbors/tests/test_neighbors.py::test_radius_neighbors_returns_array_of_objects
        - sklearn/neighbors/tests/test_neighbors.py::test_kneighbors_regressor_sparse
sklearn.utils
    utils.extmath.randomized_svd
        - sklearn/utils/tests/test_extmath.py::test_randomized_svd_low_rank_all_dtypes
        - sklearn/utils/tests/test_extmath.py::test_randomized_svd_sparse_warnings

The following are the places where "sparse matrix" option is used in parameter validation:

Details

One major reason why so many things are tested yet so few errors are raised without changing to "sparse array" is that, "array-like" seems to be covering sparse matrices and sparse arrays since they have the shape and __len__ attributes. I'm thinking... should "array-like" not include these sparse containers (for instance, by adding and not sparse.issparse(val))? @jjerphan

Also @glemaitre who posted a comment #27317 (comment) in the original PR and @jeremiedbb who is the main author of _param_validation.py.

github-actions · 2023-12-12T15:35:59Z

✔️ Linting Passed

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

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

jjerphan

Thank you, @Charlie-XIAO.

jjerphan · 2023-12-13T06:02:53Z

sklearn/cluster/_optics.py

 @validate_params(
    {
-        "X": [np.ndarray, "sparse matrix"],
+        "X": [np.ndarray, "sparse container"],


I think I would keep such strings unchanged and consider their change in another PR since they touch the public API. I would wait for Jeremy, because he is the one who has the authority on it.

This applies here, and in other places as well.

@jjerphan In fact tests would fail without these changes, unless I do not change the validation for "sparse matrix". This involves some confusing issues and let me make things a bit clearer. I think there are two possibilities for this PR:

Let "sparse matrix" and "sparse container" have the same validation, i.e., sparse.issparse() instead of changing the validation of "sparse matrix" to sparse.isspmatrix(). In this case I can leave the parameter validations unchanged.

Change the validation of "sparse matrix" to sparse.isspmatrix() and add the option "sparse container". This is, however, much more complicated, because we would have to touch the public API to make tests pass. As a matter of fact, I would expect much more tests to fail with this change (meaning that more parameter validations need to be changed to make things pass). However, only a few tests failed for the following reason: "array-like" is covering sparse matrices and sparse arrays, so that if a function/class accepts both "array-like" and "sparse matrix" then there will be no error. In other words, with the current validation system, ["array-like", "sparse matrix"] is the same as ["array-like"], which I do not think is reasonable. To take this option, we may need to: (1) change "array-like" to not accept sparse stuff; (2) with the updated "sparse matrix" validation, find out all tests that are failing (I would expect a lot), and from those failing tests I think we can know which functions/classes have been tests on sparse arrays, and we can change the constraints from "sparse matrix" to "sparse container".

I'm actually not sure which option to take. Option 1 is somehow doing nothing, in which case I think we can just discard this PR. Option 2 is more reasonable, i.e., this PR is not only to extend tests but also to change the public API and fix parameter validations, but perhaps it is too much. Well, perhaps we can first make a PR to fix the problem of "array-like" covering sparse matrices and sparse arrays (if this is indeed not the desired behavior), then do the rest of Option 2 in this PR with the title changed. I do want to know what maintainers think, also @jeremiedbb.

I think sparse containers are considered to be array-like. So option 2 works for me, but I would wait for the opinion of another maintainer.

Charlie-XIAO · 2024-01-10T17:29:08Z

I am trying to draw attention from maintainers and move the discussion to a more visible place, so I opened #28099.

jeremiedbb · 2024-02-28T14:59:31Z

As explained in #28101 (comment), I think we should not introduce a separate sparse container constraint. Let's keep a single sparse matrix constraint for now (like we left sparse matrix in all our docstrings for now) and maybe at some point we'll rename it sparse container everywhere.

I extracted the addition of test for sparse containers and added it to #28101 so I think we can close this PR.

cc/ @glemaitre

Charlie-XIAO · 2024-02-28T15:04:28Z

Just to add some thoughts: I think it's reasonable to close, in the sense that: though having two constraints can help us find which functions/classes have been tested against sparse arrays and which are not, it may convey a wrong message to users that "sparse array" is intentionally not supported (while the fact is that it is simply not tested).

jeremiedbb · 2024-02-28T15:12:34Z

though having two constraints can help us find which functions/classes have been tested against sparse arrays and which are not,

That's interesting. I think one day we'll probably want to deprecate sparse matrices and keep only sparse arrays. I expect that it will reveal everything that was not properly tested against sparse arrays. And the things we'll miss will probably be the functions that we already don't test against sparse matrices although we should, but having 2 constraints can't help us there.

glemaitre · 2024-02-28T16:57:25Z

I agree that we can close this PR and revisit the concept of sparse array when they actually behave as numpy array.

Charlie-XIAO added 3 commits November 22, 2023 22:16

TST extend tests for sklearn/utils/tests/test_param_validation

415f14c

Merge remote-tracking branch 'upstream/main' into tst-sp-param-val

0d2903f

sparse matrix -> sparse container where tests raise errors

547c1a0

github-actions bot added module:cluster module:neighbors module:utils labels Dec 12, 2023

Charlie-XIAO changed the title ~~Tst sp param val~~ TST Extend tests for scipy.sparse.*array in sklearn/utils/tests/test_param_validation.py Dec 12, 2023

Merge remote-tracking branch 'upstream/main' into tst-sp-param-val

63dbdb3

Charlie-XIAO marked this pull request as ready for review December 13, 2023 03:10

Charlie-XIAO mentioned this pull request Dec 13, 2023

TST Extend tests for scipy.sparse.*array in sklearn/utils/tests/test_param_validation.py #27317

Closed

jjerphan added the No Changelog Needed label Dec 14, 2023

jjerphan reviewed Dec 14, 2023

View reviewed changes

marenwestermann added Waiting for Second Reviewer First reviewer is done, need a second one! Waiting for Reviewer and removed Waiting for Second Reviewer First reviewer is done, need a second one! labels Jan 5, 2024

Charlie-XIAO mentioned this pull request Jan 10, 2024

BUG "array-like" in parameter validation treats sparse containers as valid inputs #28099

Closed

This was referenced Feb 19, 2024

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

Closed

convert sparse matrix into sparse container (TST Extend tests for scipy.sparse.*array ) #28456

Closed

jeremiedbb mentioned this pull request Feb 28, 2024

FIX make 'array-like' reject sparse containers in parameter validation #28101

Merged

glemaitre closed this Feb 28, 2024

Charlie-XIAO deleted the tst-sp-param-val branch February 28, 2024 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

TST Extend tests for `scipy.sparse.*array` in `sklearn/utils/tests/test_param_validation.py`#27950

TST Extend tests for `scipy.sparse.*array` in `sklearn/utils/tests/test_param_validation.py`#27950
Charlie-XIAO wants to merge 4 commits intoscikit-learn:mainfrom
Charlie-XIAO:tst-sp-param-val

Charlie-XIAO commented Dec 12, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Dec 12, 2023 •

edited

Loading

Uh oh!

jjerphan left a comment

Uh oh!

jjerphan Dec 13, 2023 •

edited

Loading

Uh oh!

Charlie-XIAO Dec 14, 2023

Uh oh!

jjerphan Dec 16, 2023

Uh oh!

Charlie-XIAO commented Jan 10, 2024 •

edited

Loading

Uh oh!

jeremiedbb commented Feb 28, 2024

Uh oh!

Charlie-XIAO commented Feb 28, 2024 •

edited

Loading

Uh oh!

jeremiedbb commented Feb 28, 2024 •

edited

Loading

Uh oh!

glemaitre commented Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

✔️ Linting Passed

Uh oh!

jjerphan left a comment

Choose a reason for hiding this comment

Uh oh!

jjerphan Dec 13, 2023 • 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 Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

jjerphan Dec 16, 2023

Choose a reason for hiding this comment

Uh oh!

Charlie-XIAO commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeremiedbb commented Feb 28, 2024

Uh oh!

Charlie-XIAO commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeremiedbb commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glemaitre commented Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Charlie-XIAO commented Dec 12, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023 •

edited

Loading

jjerphan Dec 13, 2023 •

edited

Loading

Charlie-XIAO commented Jan 10, 2024 •

edited

Loading

Charlie-XIAO commented Feb 28, 2024 •

edited

Loading

jeremiedbb commented Feb 28, 2024 •

edited

Loading