TST Extend tests for `scipy.sparse.*array` in `sklearn/utils/_testing.py` by y-vectorfield · Pull Request #27847 · scikit-learn/scikit-learn

y-vectorfield · 2023-11-26T08:17:06Z

Towards #27090.

github-actions · 2023-11-26T08:18:29Z

✔️ Linting Passed

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

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

jjerphan

Hi @y-vectorfield,

Thank you for this contribution. The tests can't be run because this file only contains test fixtures.

jjerphan · 2023-11-26T10:22:40Z

sklearn/utils/_testing.py

        )
    elif constructor_name == "sparse_csc":
-        return sp.sparse.csc_matrix(container, dtype=dtype)
+        return csr_container(dtype=dtype)


Same suggestions as above but for csc instead of csr.

I returned the original code.

Suggested change

return csr_container(dtype=dtype)

return sp.sparse.csc_matrix(container, dtype=dtype)

jjerphan · 2023-11-26T10:23:17Z

sklearn/utils/_testing.py

        os.unlink(source_file)


+@pytest.mark.parametrize("csr_container", CSR_CONTAINERS)


This is a fixture, not a test, so it need not be parametrized.

Suggested change

@pytest.mark.parametrize("csr_container", CSR_CONTAINERS)

I removed this code.

jjerphan · 2023-11-26T10:28:56Z

sklearn/utils/_testing.py

@@ -781,15 +783,15 @@ def _convert_container(
    elif constructor_name == "slice":
        return slice(container[0], container[1])
    elif constructor_name == "sparse_csr":


Suggested change

elif constructor_name == "sparse_csr":

elif constructor_name == "sparse_csr":

# TODO: when we depend on SciPy>=1.8 return `csr_array` instead and change

# the case bellow from `sparse_csr_array` to `sparse_csr_matrix` and make

# it return a `csr_matrix`.

I added your suggestion to the source file.

jjerphan · 2023-11-26T10:29:17Z

sklearn/utils/_testing.py

        return slice(container[0], container[1])
    elif constructor_name == "sparse_csr":
-        return sp.sparse.csr_matrix(container, dtype=dtype)
+        return csr_container(dtype=dtype)


Suggested change

return csr_container(dtype=dtype)

return sp.sparse.csr_matrix(container, dtype=dtype)

I also returned the original code.

jjerphan · 2023-11-26T10:30:08Z

sklearn/utils/_testing.py

 )
 from sklearn.utils._array_api import _check_array_api_dispatch
-from sklearn.utils.fixes import parse_version, sp_version
+from sklearn.utils.fixes import CSR_CONTAINERS, parse_version, sp_version


This is not needed anymore.

Suggested change

from sklearn.utils.fixes import CSR_CONTAINERS, parse_version, sp_version

from sklearn.utils.fixes import parse_version, sp_version

I also removed this module import.

y-vectorfield · 2023-11-26T13:01:20Z

@jjerphan, thank you very much for your comments.
I did not need to modify this source, by any chance??

jjerphan · 2023-11-26T13:58:20Z

I have added a few suggestions in my review above to show what needs to be modified.

jjerphan

Ignore this second review: I was confused by your commit, please do not force push after a first review: this makes keeping track of changes harder.

y-vectorfield · 2023-11-26T21:32:33Z

Ignore this second review: I was confused by your commit, please do not force push after a first review: this makes keeping track of changes harder.

Oh, sorry, I will be careful not to force push in the future.

jjerphan · 2023-11-27T12:30:43Z

Oh, sorry, I will be careful not to force push in the future.

It's fine. :)

I will review the PR once the failures on the CI are fixed. Let me know if you need help.

y-vectorfield · 2023-11-28T15:34:57Z

Oh, sorry, I will be careful not to force push in the future.

It's fine. :)

I will review the PR once the failures on the CI are fixed. Let me know if you need help.

Hello, @jjerphan, thank you very much for your message.
I would like to teach what I should do.
I returned the original codes and added only comments.

jjerphan · 2023-11-29T21:16:20Z

sklearn/utils/_testing.py

        )
    elif constructor_name == "sparse_csc":
-        return sp.sparse.csc_matrix(container, dtype=dtype)
+        return csr_container(dtype=dtype)


This needs to be changed as suggested above.

Hello, @jjerphan,
Oh, I'm sorry. Do you mean as follows??

current version

return csr_container(dtype=dtype)

modified version(I will add.)

return csc_container(dtype=dtype)

The current version of source is as follow,
https://github.com/y-vectorfield/scikit-learn/blob/1d45c34d1f2706fbdd5599644f317e4a3a34934f/sklearn/utils/_testing.py#L761C1-L788C60

I meant #27847 (comment) and #27847 (comment) but for CSC.

@jjerphan, I'm sorry for the confusion.
I modified the source.

jjerphan

What I meant is shown by those suggestions.

jjerphan · 2023-12-02T11:40:41Z

sklearn/utils/_testing.py

    elif constructor_name == "sparse_csr_array":
        if sp_version >= parse_version("1.8"):
-            return sp.sparse.csr_array(container, dtype=dtype)
+            return sp.sparse.csc_array(container, dtype=dtype)


Suggested change

return sp.sparse.csc_array(container, dtype=dtype)

return sp.sparse.csr_array(container, dtype=dtype)

@y-vectorfield you forgot this one.

@jjerphan ,
Oh, I'm sorry. I reflected your comment.

jjerphan · 2023-12-02T11:41:25Z

sklearn/utils/_testing.py

        raise ValueError(
            f"sparse_csr_array is only available with scipy>=1.8.0, got {sp_version}"
        )
    elif constructor_name == "sparse_csc":


Suggested change

elif constructor_name == "sparse_csc":

elif constructor_name == "sparse_csc":

# TODO: when we depend on SciPy>=1.8 return `csc_array` instead and change

# the case bellow from `sparse_csc_array` to `sparse_csc_matrix` and make

# it return a `csc_matrix`.

jjerphan · 2023-12-02T11:42:23Z

sklearn/utils/_testing.py

        )
    elif constructor_name == "sparse_csc":
-        return sp.sparse.csc_matrix(container, dtype=dtype)
+        return csr_container(dtype=dtype)


Suggested change

return csr_container(dtype=dtype)

return sp.sparse.csc_matrix(container, dtype=dtype)

jjerphan · 2023-12-09T15:53:36Z

@y-vectorfield: are you still interested in working on this PR?

y-vectorfield · 2023-12-10T06:27:13Z

@y-vectorfield: are you still interested in working on this PR?

Hello, @jjerphan, thank you for your message.
Yes, I will work on this PR. Please wait for a moment.

marenwestermann · 2024-02-01T10:50:33Z

Hi @y-vectorfield would you like to continue working on this PR? Let us know if you need help.

y-vectorfield · 2024-02-03T00:27:40Z

Hello, @marenwestermann , very sorry. I've been away from this task for a while.
I will submit a PR in the next few days.
Thank you!

y-vectorfield · 2024-02-19T06:25:26Z

@jjerphan , @marenwestermann ,
Sorry for the late modification.
I reflected on your comments.
If the review is finished, I will rebase this branch.

y-vectorfield · 2024-02-21T04:37:26Z

@jjerphan , @marenwestermann ,
If there is no problem, please approve this PR.
I will rebase this branch.

Charlie-XIAO · 2024-02-21T05:40:20Z

I didn't get what this PR intends to do. It seems that on main we already have support for returning sparse arrays, added in an earlier PR #27276 when extending tests for utils/tests/test_testing.py.

jjerphan · 2024-02-21T09:10:48Z

There are still a couple of adaption left for sparse array and sparse matrices.

I do not have time to sort this out, but it should be trivial.

Can someone have a look at the merge conflicts, please?

Charlie-XIAO · 2024-02-21T11:59:15Z

scikit-learn/sklearn/utils/_testing.py

Lines 815 to 836 in f323eb4

    
           elif "sparse" in constructor_name: 
        
               if not sp.sparse.issparse(container): 
        
                   # For scipy >= 1.13, sparse array constructed from 1d array may be 
        
                   # 1d or raise an exception. To avoid this, we make sure that the 
        
                   # input container is 2d. For more details, see 
        
                   # https://github.com/scipy/scipy/pull/18530#issuecomment-1878005149 
        
                   container = np.atleast_2d(container) 
        
               if "array" in constructor_name and sp_version < parse_version("1.8"): 
        
                   raise ValueError( 
        
                       f"{constructor_name} is only available with scipy>=1.8.0, got " 
        
                       f"{sp_version}" 
        
                   ) 
        
               if constructor_name in ("sparse", "sparse_csr"): 
        
                   # sparse and sparse_csr are equivalent for legacy reasons 
        
                   return sp.sparse.csr_matrix(container, dtype=dtype) 
        
               elif constructor_name == "sparse_csr_array": 
        
                   return sp.sparse.csr_array(container, dtype=dtype) 
        
               elif constructor_name == "sparse_csc": 
        
                   return sp.sparse.csc_matrix(container, dtype=dtype) 
        
               elif constructor_name == "sparse_csc_array": 
        
                   return sp.sparse.csc_array(container, dtype=dtype)

The problem is that I tried to resolve the merge conflicts but then no relevant changes are left. As above we already have csc and csr arrays and matrices on main, and this PR does not seem to involve anything beyond that. Or should it?

jjerphan · 2024-02-21T12:47:44Z

OK, so then let's close it.

y-vectorfield · 2024-02-21T13:49:28Z

@jjerphan , thank you very much for your review.
Unfortunately, the corrections could not be reflected this time.
I will help you the next time.

github-actions bot added the module:utils label Nov 26, 2023

y-vectorfield mentioned this pull request Nov 26, 2023

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

Closed

jjerphan reviewed Nov 26, 2023

View reviewed changes

TST Extend tests for scipy.sparse.*array in sklearn/utils/_testing.py

1d45c34

y-vectorfield force-pushed the yv_2311_extends_some_tests_for_scipy_sparse_arrays branch from 0f4abb3 to 1d45c34 Compare November 26, 2023 13:02

jjerphan reviewed Nov 26, 2023

View reviewed changes

y-vectorfield marked this pull request as ready for review November 26, 2023 21:41

jjerphan reviewed Nov 29, 2023

View reviewed changes

y-vectorfield added 3 commits December 2, 2023 07:35

Add scipy sparse csc instead of csr

028bc4d

Modify source

21f1ff4

Add maintainer's comment

40933be

jjerphan reviewed Dec 2, 2023

View reviewed changes

Reflect review comments

63e265b

Refect additional comment

2dfec12

jjerphan closed this Feb 21, 2024

	return csr_container(dtype=dtype)
	return sp.sparse.csc_matrix(container, dtype=dtype)

		os.unlink(source_file)


		@pytest.mark.parametrize("csr_container", CSR_CONTAINERS)

-    elif constructor_name == "sparse_csr":
+    elif constructor_name == "sparse_csr":
+        # TODO: when we depend on SciPy>=1.8 return `csr_array` instead and change
+        # the case bellow from `sparse_csr_array` to `sparse_csr_matrix` and make
+        # it return a `csr_matrix`.

	return csr_container(dtype=dtype)
	return sp.sparse.csr_matrix(container, dtype=dtype)

	from sklearn.utils.fixes import CSR_CONTAINERS, parse_version, sp_version
	from sklearn.utils.fixes import parse_version, sp_version

	return sp.sparse.csc_array(container, dtype=dtype)
	return sp.sparse.csr_array(container, dtype=dtype)

-    elif constructor_name == "sparse_csc":
+    elif constructor_name == "sparse_csc":
+        # TODO: when we depend on SciPy>=1.8 return `csc_array` instead and change
+        # the case bellow from `sparse_csc_array` to `sparse_csc_matrix` and make
+        # it return a `csc_matrix`.

Uh oh!

Conversation

y-vectorfield commented Nov 26, 2023

Uh oh!

github-actions bot commented Nov 26, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjerphan Dec 2, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

y-vectorfield commented Nov 26, 2023

Uh oh!

jjerphan commented Nov 26, 2023

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!

y-vectorfield commented Nov 26, 2023

Uh oh!

jjerphan commented Nov 27, 2023

Uh oh!

y-vectorfield commented Nov 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

y-vectorfield Nov 29, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjerphan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

y-vectorfield Feb 19, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

jjerphan Dec 2, 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!

jjerphan commented Dec 9, 2023

Uh oh!

github-actions bot commented Nov 26, 2023 •

edited

Loading

jjerphan Dec 2, 2023 •

edited

Loading

jjerphan left a comment •

edited

Loading

y-vectorfield Nov 29, 2023 •

edited

Loading

y-vectorfield Feb 19, 2024 •

edited

Loading

jjerphan Dec 2, 2023 •

edited

Loading

y-vectorfield commented Dec 10, 2023 •

edited

Loading

Charlie-XIAO commented Feb 21, 2024 •

edited

Loading