8000 TST Extend tests for `scipy.sparse.*array` in `sklearn/utils/tests/test_param_validation.py` by naman1608 · Pull Request #27317 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

naman1608
Copy link
Contributor

Reference Issues/PRs

Towards #27090.

What does this implement/fix? Explain your changes.

Any other comments?

@github-actions
Copy link
github-actions bot commented Sep 8, 2023 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: 447a467. Link to the linter CI: here

@naman1608 naman1608 changed the title sparse matrix to csr containers TST Extend tests for scipy.sparse.*array in sklearn/utils/tests/test_param_validation.py Sep 8, 2023
@naman1608
Copy link
Contributor Author

Could someone please help why the build is failing

@jjerphan jjerphan mentioned this pull request Sep 9, 2023
Closed
@glemaitre
Copy link
Member

@jeremiedbb I would be inclined at not changing this part of the code for the moment.
We are currently not covering all possible sparse type so I don't think that we want to check all type of sparse container.

Maybe we could in the future generate both a sparse array and a sparse matrix.

jjerphan
jjerphan reviewed Nov 1, 2023
View reviewed changes
sklearn/utils/_param_validation.py
@@ -850,7 +850,7 @@ def generate_valid_param(constraint):
if isinstance(constraint, _ArrayLikes):
return np.array([1, 2, 3])

if isinstance(constraint, _SparseMatrices):
if isinstance(constraint, _SparseContainers):
return csr_matrix([[0, 1], [1, 0]])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return csr_matrix([[0, 1], [1, 0]])
return csr_array([[0, 1], [1, 0]])

sklearn/utils/_param_validation.py
Comment on lines +117 to +118
if isinstance(constraint, str) and constraint == "sparse container":
return _SparseContainers()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to change all the occurrences of "sparse matrix" to "sparse container" for input parameters' validation. Doc-strings also need to be changed accordingly.

@jjerphan
Copy link
Member

Hi @naman1608,

Do you still have time to finish this contribution?

@jjerphan
Copy link
Member

@Charlie-XIAO: are you interested in continuing this work?

@Charlie-XIAO
Copy link
Contributor

@jjerphan Okay sure, I will take a look later.

@Charlie-XIAO
Copy link
Contributor

@jjerphan Btw, I want to make sure whether we do want to change all occurrences of "sparse matrix" into "sparse container", given #27317 (comment)? Or do we change only for those that have been tested while still keeping the "sparse matrix" option?

@jjerphan
Copy link
Member

Or do we change only for those that have been tested while still keeping the "sparse matrix" option?

I think this is the best option. We probably want to discuss changing "sparse matrix" to "sparse container" in a dedicated PR since this is would touch the input parameter validation.

@jjerphan
Copy link
Member
jjerphan commented Dec 9, 2023

Hello @Charlie-XIAO, are you still interested in continuing this PR?

@Charlie-XIAO
Copy link
Contributor

@jjerphan Yes I am. I’m extremely sorry for the delay because I was working on a capstone and some exams in the past few weeks. I will be able to work on it soon (within a week).

@jjerphan
Copy link
Member

Hi @Charlie-XIAO,

Take your time: it is fine, really. I just wanted to know in case someone else new to the project could have started with a contribution like this.

Good luck with your exams!

@Charlie-XIAO Charlie-XIAO mentioned this pull request Dec 12, 2023
Closed
69 tasks
@Charlie-XIAO
Copy link
Contributor

@jjerphan Hi I finally had time to work on this, please check #27950. I believe there is something strange happening with parameter validation which I explained in that PR. In short, "array-like" seems to be covering sparse arrays and sparse matrices which does not seem reasonable.
0