Skip to content
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

maint: Adapt to breaking changes in scipy.sparse.isspmatrix #598

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

pfebrer
Copy link
Contributor

@pfebrer pfebrer commented Jul 13, 2023

In this PR: scipy/scipy#18528 scipy introduced a breaking change which has already been released in 1.11.

isspmatrix no longer returns True if you pass a sparse array (instead of a sparse matrix).

To keep the old behavior, we should use scipy.sparse.issparse instead. isspmatrix_fmt (e.g. isspmatrix_csr) becomes more cumbersome because there is no equivalent function in scipy to check for sparse arrays as well, and you have to go issparse(matrix) and matrix.format == "fmt".

These changes keep the old behavior of sisl even with scipy >= 1.11 and don't introduce backwards incompatibilities, so no need to bump the scipy dependency (I think).

This used to return True with sparse arrays as well, but now it only
returns True for sparse matrices.

To keep the old behavior, we use `scipy.sparse.issparse` instead.
@@ -65,7 +65,7 @@
sort,
zeros,
)
from scipy.sparse import csr_matrix, hstack, identity, isspmatrix
from scipy.sparse import csr_matrix, hstack, identity, issparse

Check notice

Code scanning / CodeQL

Unused import

Import of 'identity' is not used.
@zerothi
Copy link
Owner

zerothi commented Jul 13, 2023

this won't work for older scipy's, you need to check scipy version before doing this, preferably importing into a unified name depending on version.

@zerothi
Copy link
Owner

zerothi commented Jul 13, 2023

Also, all of sisl still uses sparse matrices, but I agree it would be nice if users could do sparse arrays.

@zerothi
Copy link
Owner

zerothi commented Jul 13, 2023

Note this could be collected into a sisl._scipy_imports package to make it simpler.

@pfebrer
Copy link
Contributor Author

pfebrer commented Jul 13, 2023

I have an application that uses sisl with arrays, that's how I noticed the bug.

I checked up to the minimum version that sisl supports now (1.5.0), and issparse was already there.

@zerothi
Copy link
Owner

zerothi commented Jul 13, 2023

Sorry, I missed it!

Copy link
Owner

@zerothi zerothi left a comment

Choose a reason for hiding this comment

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

Thanks!

@zerothi zerothi merged commit 708e494 into zerothi:main Jul 13, 2023
@zerothi
Copy link
Owner

zerothi commented Jul 13, 2023

FYI I could also locate it in 0.10, so I think we are safe! Thanks!

@pfebrer
Copy link
Contributor Author

pfebrer commented Jul 13, 2023

Perfect :)

@pfebrer
Copy link
Contributor Author

pfebrer commented Jul 13, 2023

By the way I think now the isspmatrix_csr, coo, etc... in sisl.sparse are unused. Just in case you want to remove them

@zerothi
Copy link
Owner

zerothi commented Jul 13, 2023

yeah, they should just be removed.

@zerothi
Copy link
Owner

zerothi commented Jul 13, 2023

These got corrected, now a little more to go and we are done! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants