-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
BUG: sparse.csgraph, array types: support non-zero fill_value
s
#20773
Conversation
I think once |
fill_value
s in csgraph
fill_value
s in csgraph
fill_value
s in csgraph
fill_value
s
The tests at |
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 work on this @mtsokol!
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.
The test and changes LGTM, except for relying on a feature that is brand new in pydata sparse. How about wrapping the to_scipy_sparse(accept_fv)
call in a try-except, catching the TypeError
and then callign again without use of accept_fv
? That keeps the default behavior with 0
fill value supported for older versions.
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.
A few more places where a new sparse
version is required.
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 all the hard work here @mtsokol!
Co-authored-by: Hameer Abbasi <2190658+hameerabbasi@users.noreply.github.com>
Some comments above each try-except would be useful. I suggest: # The `accept_fv` keyword is new in PyData Sparse 0.15.4 (May 2024),
# remove the `except` once the minimum supported version is >=0.15.4 |
Sure! I added comments. |
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 now, let's give this a go. Thanks @mtsokol, and thanks @hameerabbasi for the reviews.
It looks like the new code doesn't handle |
Connected to: #20763, pydata/sparse#685, finch-tensor/finch-tensor-python#59
Hi @hameerabbasi,
This PR allows to pass
pydata/sparse
arrays withnp.inf
fill-values to the selectedscipy.sparse.csgraph
functions.