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

Add a sum method to _sparse_py_operators #745

Merged
merged 2 commits into from
Jan 12, 2022

Conversation

aerubanov
Copy link
Contributor

This PR add sum method for sparse tensors according to #522. So one can call x.sum() instead of aesara.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.

@brandonwillard brandonwillard added enhancement New feature or request sparse tensors labels Jan 12, 2022
Copy link
Member

@brandonwillard brandonwillard left a 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 asserts 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.

@brandonwillard brandonwillard changed the title add sum method to _sparse_py_operators Add a sum method to _sparse_py_operators Jan 12, 2022
@codecov
Copy link

codecov bot commented Jan 12, 2022

Codecov Report

Merging #745 (8e30d88) into main (3edbbc4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
aesara/sparse/basic.py 82.11% <100.00%> (+0.02%) ⬆️
aesara/graph/opt.py 65.74% <0.00%> (ø)
aesara/tensor/type_other.py 82.75% <0.00%> (+1.98%) ⬆️

@aerubanov
Copy link
Contributor Author

aerubanov commented Jan 12, 2022

I just deleted this strange assert.

Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@brandonwillard brandonwillard merged commit 51792fe into aesara-devs:main Jan 12, 2022
@brandonwillard
Copy link
Member

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.

@aerubanov aerubanov deleted the sum_sparse_method branch January 13, 2022 06:22
@brandonwillard brandonwillard linked an issue Jan 13, 2022 that may be closed by this pull request
@aerubanov
Copy link
Contributor Author

@brandonwillard, Thanks for the information! I'll be sure to take a look at that pool-request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request sparse tensors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sum to _sparse_py_operators
2 participants