Opened 13 years ago
Closed 13 years ago
#7728 closed defect (fixed)
Make matrix factorizations immutable and cached
Reported by: | dagss | Owned by: | William Stein |
---|---|---|---|
Priority: | major | Milestone: | sage-4.3.1 |
Component: | linear algebra | Keywords: | |
Cc: | Merged in: | sage-4.3.1.alpha0 | |
Authors: | Dag Sverre Seljebotn | Reviewers: | William Stein |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
The attached patch (based on 4.3.rc0):
- changes Cholesky (all/most matrices) and SVD, QR and LU factorizations (double_dense only) so that the returned resulting matrices are immutable
- adds caching for Cholesky, SVD and QR
- adds pickle-able caching for LU (and it is likely there was a a bug with pickling/unpickling a matrix with a cached factorization which this patch fixes...)
- improves doctests for SVD and QR (I wanted to more easily check that my changes didn't cause regressions...)
- adds methods
zero_at
andround
to dense double matrices (used in said doctests)
I hope the doctest improvements can be accepted as part of the patch even if I didn't bother to split it up.
Note that when dealing with matrix factorization doctesting, just avoiding 0 in the input goes very far with creating readable tests.
Attachments (2)
Change History (8)
Changed 13 years ago by
Attachment: | trac_7728-immutablefactors.patch added |
---|
comment:1 Changed 13 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 13 years ago by
Status: | needs_review → needs_work |
---|
Changed 13 years ago by
Attachment: | trac_7728-immutablefactors.2.patch added |
---|
comment:3 Changed 13 years ago by
Status: | needs_work → needs_review |
---|
Last attachment contains the same changes + a little more docs.
LU was previously cached, so this just makes things more consistent by caching all decompositions -- easier to remember.
I also plan to make use of the caching if I get around to fixing #4932, see my comment there, short story: solve_left
should be able to use the results of a call to LU()
, which makes caching a lot more important.
comment:4 follow-up: 5 Changed 13 years ago by
Status: | needs_review → positive_review |
---|
Bravo, this is an awesome patch! Positive review.
Comment for future work by somebody. I don't like this:
987 U, S, V -- immutable matrices such that $A = U*S*V.conj().transpose()$ 988 where U and V are orthogonal and S is zero off of the diagonal.
It's not your fault -- it was like that before. But it is wrong in so many ways wrt to Sphinx (e.g., dollar signs? V.conj().transpose() in math mode? variables should be listed separately, etc.
comment:5 Changed 13 years ago by
Replying to was:
It's not your fault -- it was like that before. But it is wrong in so many ways wrt to Sphinx (e.g., dollar signs? V.conj().transpose() in math mode? variables should be listed separately, etc.
Just a small comment: I believe that we can now use dollar signs for math input in Sphinx, so that shouldn't be a problem.
comment:6 Changed 13 years ago by
Merged in: | → sage-4.3.1.alpha0 |
---|---|
Resolution: | → fixed |
Reviewers: | → William Stein |
Status: | positive_review → closed |
OK, this likely need some more docs about the caching aspect...