Ticket #5554 (closed enhancement: fixed)
[with patch, positive review] implement cholesky_decomposition for matrices other than RDF
| Reported by: | ncalexan | Owned by: | was |
|---|---|---|---|
| Priority: | major | Milestone: | sage-4.0 |
| Component: | linear algebra | Keywords: | cholesky decomposition RDF real |
| Cc: | jkantor, wstein | Work issues: | |
| Report Upstream: | Reviewers: | ||
| Authors: | Merged in: | ||
| Dependencies: | Stopgaps: |
Description
The attached patch
- renames cholesky() to cholesky_decomposition()
- improves the documentation
- adds a helper function _cholesky_decomposition_() for specialization;
- moves the GSL/scipy cholesky function to matrix_real_double_dense, where it belongs;
- implements a generic Cholesky decomposition that works for arbitrary precision real and complex fields, where we generalize symmetric to Hermitian.
Attachments
Change History
comment:1 Changed 4 years ago by ncalexan
- Cc jkantor, wstein added
- Summary changed from implement cholesky_decomposition for matrices other than RDF to [with patch, needs review] implement cholesky_decomposition for matrices other than RDF
comment:3 in reply to: ↑ 2 Changed 4 years ago by rbeezer
- Summary changed from [with patch, needs review] implement cholesky_decomposition for matrices other than RDF to [with patch, needs work] implement cholesky_decomposition for matrices other than RDF
Nicely done, great documentation, passes all tests. Suggestions for improvements, in order of perceived importance.
- Inputting a matrix over ZZ or QQ fails with the ValueError message about not being symmetric or positive definite, which is not an accurate error message in this case. Should there be an attempt to coerce the matrix to RDF or CDF? Inputting (trivial) symbolic matrices proceeds successfully (eg. x,y],[y,x?). Perhaps a coercion attempt above will cause a failure in this case, and this might be the right result? Another interesting input is matrix(2, [2, 2+i,2-i,4]) which is considered to be defined over SR, yet is handled properly by the routines here, or can be convrted to CDF for an accurate numerical result.
- I think it would be safer to force the user to me more aware of the positive definite requirement. I'd prefer to see a keyword argument like "positive_definite=False" added. In other words, the default operation is to perform a check for positive-definiteness, but when the user knows the matrices at hand are positive-definite, the keyword can be invoked to bypass the check. I think there are other examples like this in Sage (but I can't locate a good one at the moment).
- A couple of places in the documentation could benefit from including the greater generality of matrices over the complex numbers, which this enhancement now allows. Specifically:
(a) docstring in matrix2.pyx could make it clear that L^t is the adjoint when the matrix has complex entries (rather than looking like just the transpose). (b) two error messages state that the matrix is "not symmetric." Could they say instead "not symmetric (or not Hermitian)"?
- Computing eigenvalues for matrices over a RealField() gives the warning message
UserWarning: Using generic algorithm for an inexact ring, which will probably give incorrect results due to numerical precision issues.
Would something similar be a good idea here for the generic algorithm?
comment:4 Changed 4 years ago by rbeezer
- Summary changed from [with patch, needs work] implement cholesky_decomposition for matrices other than RDF to [with patch, positive review] implement cholesky_decomposition for matrices other than RDF
Re (1): ncalexan says: I think they should fail if you can't extract a square root. And I *don't* think we should coerce anything. Ever.
Improvements 2, 3, and 4 have been moved to #6003. So this now has a positive review.
comment:5 Changed 4 years ago by mabshoff
- Summary changed from [with patch, positive review] implement cholesky_decomposition for matrices other than RDF to [with patch, needs work] implement cholesky_decomposition for matrices other than RDF
This needs work: cholesky() cannot just be deleted, it needs to be deprecated first.
The code looks very nice otherwise :)
Cheers,
Michael
comment:6 Changed 4 years ago by ncalexan
- Summary changed from [with patch, needs work] implement cholesky_decomposition for matrices other than RDF to [with patch, positive review] implement cholesky_decomposition for matrices other than RDF
Okay, this adds back cholesky, with a deprecation warning. I'm returning this to positive review, since nothing else has changed.

