-
-
Notifications
You must be signed in to change notification settings - Fork 152
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
Add a sum
method to _sparse_py_operators
#745
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.
Oddly, there's a test that assert
s the lack of a sum
method. That's what's causing the errors in CI.
Don't forget that you can run just the sparse tests locally (e.g. pytest tests/sparse
) if you want to debug; those shouldn't take too long to run by themselves.
sum
method to _sparse_py_operators
Codecov Report
@@ Coverage Diff @@
## main #745 +/- ##
=======================================
Coverage 78.16% 78.17%
=======================================
Files 152 152
Lines 47652 47663 +11
Branches 10880 10881 +1
=======================================
+ Hits 37249 37260 +11
Misses 7846 7846
Partials 2557 2557
|
I just deleted this strange assert. |
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.
Looks great, thanks!
By the way, if you're interested in revolutionizing the sparse matrix support in Aesara, take a look at #303. It's actually pretty close to done, but I haven't had the time to get back to it. You're more than welcome to take that over, though. |
@brandonwillard, Thanks for the information! I'll be sure to take a look at that pool-request. |
This PR add
sum
method for sparse tensors according to #522. So one can callx.sum()
instead ofaesara.sparse.sp_sum(x)
.I think this change should also be reflected in the documentation, but I could not find a section with the methods of sparse tensors. We need to decide something about what to do with it.