-
Notifications
You must be signed in to change notification settings - Fork 145
Reorganize the sparse
module
#1674
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
base: main
Are you sure you want to change the base?
Reorganize the sparse
module
#1674
Conversation
I think it was a mistake to inherit from TensorVariable in the first place. They are way too distinct objects |
I believe we should revert aesara-devs/aesara#142, in fact the example given of Subtensor just further proves the point. Indexing in sparsevariables has nothing to do with in indexing in tensorvariables, starting with the fact there are no sparse vectors, only matrices |
mypy is failing because of that inheritance for what its worth |
I agree. The current mixins don't make any sense at all. We have e.g. |
3d78e1e
to
2e2211c
Compare
tests/sparse/test_math.py
Outdated
else: | ||
return pytensor.sparse.matrix(format, name, dtype=dtype) | ||
|
||
params = product( |
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.
This leads to 2,304 test cases, which seems slightly excessive.
2e2211c
to
a823f30
Compare
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (81.86%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1674 +/- ##
==========================================
- Coverage 81.64% 81.62% -0.02%
==========================================
Files 242 245 +3
Lines 53573 53764 +191
Branches 9453 9451 -2
==========================================
+ Hits 43737 43887 +150
- Misses 7358 7400 +42
+ Partials 2478 2477 -1
🚀 New features to boost your workflow:
|
a823f30
to
659fec8
Compare
Description
Reorganize the existing sparse code to be a bit more logical. In particular, the sparse module now mirrors the tensor module:
sparse/basic.py
has constructors and utilitiessparse/math.py
has math stuffsparse/variable.py
has base classessparse/linalg.py
has linear algebraOther stuff was mostly untouched. My motivation here is to lay the groundwork to get people to come look at and work on sparse. Things were pretty messy before. There was a lot of magic I didn't like. For example, the
_sparse_py_operators
inherited from_tensor_py_operators
, and did some wrapper magic to dispatch a huge list of methods to sparse. I made this a lot more verbose, and putNotImplemented
everywhere, so it's obvious to potential helpers what needs to be implemented.Related Issue
Checklist
Type of change
📚 Documentation preview 📚: https://pytensor--1674.org.readthedocs.build/en/1674/