Opened 13 years ago
Last modified 5 weeks ago
#7723 needs_work enhancement
Sparse matrices for double fields
Reported by: | dagss | Owned by: | jkantor |
---|---|---|---|
Priority: | major | Milestone: | |
Component: | linear algebra | Keywords: | |
Cc: | Merged in: | ||
Authors: | Dag Sverre Seljebotn | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Here's the beginnings of my work on sparse double matrices.
This is based on 4.3.rc0. Note that I have *not* run the entire Sage test suite, only tests in the matrix package. I'm happy to run the entire suite once I know the final revision this will be rebased to, but 4.3.rc0 produces a few test failures in itself (=noise I'm not bothering with for the moment).
There are three patches, which should be applied and reviewed in this order:
- generic_multiply lets one override matrix multiplication with different parents. This is in a seperate patch because it changes structure/element.pxd, causing a big recompile.
- double_sparse is the main new classes
- coo_format changes the matrix constructor to accept "coo=..." (see docstring in patch)
More comments:
- I will not introduce seperate classes for real and complex -- there will be other subclasses (Hermitian, strictly-diagonal etc.) and I don't want to double the size of the hierarchy. There are other (and better) ways to get efficient getitem/setitem without a speed penalty (such as introducing a seperate ItemAccessor? protocol/class -- though for sparse matrices an if-test won't matter either).
- Once this is accepted (and I have a general feel for what I do right and wrong) I hope to continue with solvers etc. (as I scratch my itches).
Attachments (4)
Change History (21)
comment:1 Changed 13 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 13 years ago by
Component: | numerical → linear algebra |
---|
Changed 13 years ago by
Attachment: | trac_7723-coo_format.patch added |
---|
Changed 13 years ago by
Attachment: | trac_7723-double_sparse.patch added |
---|
Changed 13 years ago by
Attachment: | trac_7723-generic_multiply.patch added |
---|
comment:3 Changed 13 years ago by
Status: | needs_review → needs_work |
---|
comment:4 follow-up: 5 Changed 13 years ago by
Status: | needs_work → needs_review |
---|
The attached patch should address the above issues (I guess this should have been folded into the original patches? But I hope this is OK, I still have some way to go before getting to terms with using Mercurial/MQ in a patch-review process.)
Changed 13 years ago by
Attachment: | trac_7723-fixround1.patch added |
---|
comment:5 Changed 13 years ago by
Replying to dagss:
The attached patch should address the above issues (I guess this should have been folded into the original patches? But I hope this is OK, I still have some way to go before getting to terms with using Mercurial/MQ in a patch-review process.)
I'm really glad you *didn't* fold this into the original patch, since it makes it much easier for me to see what you changed.
comment:7 Changed 13 years ago by
Milestone: | sage-feature → sage-4.3.1 |
---|
comment:9 Changed 13 years ago by
I can't reproduce this, applying on 4.3.1.alpha0.
Did you apply them in the wrong order? Apparently I messed up the order when uploading, sorry. The patches must be applied in this order:
trac_7723-generic_multiply.patch trac_7723-double_sparse.patch trac_7723-coo_format.patch trac_7723-fixround1.patch
comment:11 Changed 13 years ago by
Status: | positive_review → needs_work |
---|
The following tests failed: sage -t -long devel/sage-main/sage/matrix/constructor.py # 1 doctests failed sage -t -long devel/sage-main/sage/categories/action.pyx # Segfault
comment:12 Changed 13 years ago by
Ouch.
I have not idea when I can get back to this at the moment. Basically what has happened is that I bit the bullet and implemented my own numerical matrix class hierarchy which is usable without Sage (but loosely modeled after it). That ended up giving me the results I needed much faster...
The long-term goal is to perhaps try to merge this back into Sage, however as there's no real benefit for my own work in doing that I don't really know if or when.
(Anyone who finds this ticket because they need this functionality are welcome to send me an email and check the status.)
comment:13 Changed 9 years ago by
Milestone: | sage-5.11 → sage-5.12 |
---|
comment:14 Changed 9 years ago by
Milestone: | sage-6.1 → sage-6.2 |
---|
comment:15 Changed 9 years ago by
Milestone: | sage-6.2 → sage-6.3 |
---|
comment:16 Changed 8 years ago by
Milestone: | sage-6.3 → sage-6.4 |
---|
comment:17 Changed 5 weeks ago by
Milestone: | sage-6.4 |
---|
REFEREE REPORT:
Looks good, except there is a missing word "have" in this documentation:
This doesn't cause any doctest failures, and factoring this code out is a good idea.
Looks good.
This causes a bunch of doctest failures:
I noticed a *massive* performance regression as a result of your patches:
Ouch. These are both dense matrices. There must be something that got seriously messed up with re-factoring?
matrix_double_sparse.pyx
:---
Overall I'm very happy and excited about this patch!! I'm really, really glad you're working on this, and would like to do anything I can to encourage this. It is critically valuable to the Sage project to get much better numerical sparse matrix support.