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:

Status badges

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 and round 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)

trac_7728-immutablefactors.patch (23.4 KB) - added by dagss 13 years ago.
trac_7728-immutablefactors.2.patch (24.9 KB) - added by dagss 13 years ago.

Download all attachments as: .zip

Change History (8)

Changed 13 years ago by dagss

comment:1 Changed 13 years ago by dagss

Status: newneeds_review

comment:2 Changed 13 years ago by dagss

Status: needs_reviewneeds_work

OK, this likely need some more docs about the caching aspect...

Changed 13 years ago by dagss

comment:3 Changed 13 years ago by dagss

Status: needs_workneeds_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 Changed 13 years ago by William Stein

Status: needs_reviewpositive_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 in reply to:  4 Changed 13 years ago by Alex Ghitza

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 Mike Hansen

Merged in: sage-4.3.1.alpha0
Resolution: fixed
Reviewers: William Stein
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.