-
-
Notifications
You must be signed in to change notification settings - Fork 26.2k
CI Fix scipy-dev build #28047
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.
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
CI Fix scipy-dev build #28047
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
@@ -370,7 +375,7 @@ def test_readonly_kwargs(): | |||
TypeError, | |||
"Sparse data was passed for w, but dense data is required", | |||
) | |||
for csr_container in CSR_CONTAINERS | |||
for csr_container in COO_CONTAINERS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the test is checking for an error, I prefer updating the container to use 2d data for w
.
(I did not know 1d SciPy CSR sparse matrices were a thing).
I am going to ping @jjerphan in case you have any insights on this, since you were involved in the COO 1d scipy PR review scipy/scipy#18530. Here is my summary as far as scikit-learn is concerned:
|
I think we should be explicit and create sparse arrays/matrix from 2d datastructures only, unless we precisely want to test the behavior of 1d datastructures. There aren't that many tests with this failure:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as fix. Thank you @lesteve.
I do not have enough bandwidth to have a look into it in details.
I wonder whether scipy/scipy#18530 might cause subtle changes or bugs for scikit-learn due to the previously implicit promotion to 2 dimensions.
I think this change of behavior has to be documented by SciPy if it is not reverted.
I let one of you merge if everything looks good to you.
Merging my own PR with two approvals, there you go 😅 I added a summary of the scikit-learn tests that broke on the scipy PR in scipy/scipy#18530 (comment). I think this is best to have the discussion there with the Scipy maintainers to get their feeling about it. |
Close #28011.
Apparently
scipy.sparse.csr_array
was able to be constructed from a 1d array. Not anymore so I used another container that can work with 1d array.The change is likely from scipy/scipy#18530, I need some more time to digest it.