Opened 9 years ago
Closed 9 years ago
#13045 closed defect (fixed)
Deprecate cholesky_decomposition() in favor of cholesky()
Reported by: | rbeezer | Owned by: | jason, was |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.1 |
Component: | linear algebra | Keywords: | sd40.5 |
Cc: | ddrake | Merged in: | sage-5.1.beta5 |
Authors: | Rob Beezer | Reviewers: | Dan Drake |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #12966, #13018, #11274, #13035 | Stopgaps: |
Description (last modified by )
cholesky_decomposition()
is needlessly verbose, and the existing spaghetti code with _cholesky_decomposition()
is too convoluted to unwind. Dependencies install the new cholesky()
alongside.
The old cholesky_decomposition()
is so broken that nobody should be using it anyway, thus it should be no problem to obsolete it.
Depends: #12966, #13018, #11274, #13035
Apply:
Attachments (2)
Change History (11)
Changed 9 years ago by
comment:1 Changed 9 years ago by
- Cc ddrake added
- Dependencies set to #12966, #13018, #11274, #13035
- Description modified (diff)
- Keywords sd40.5 added
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
One note: string formatting when raising an exception is, in many cases, a really bad idea, since generating the string may take a really long time. This mostly happens when trying to represent a very large number or other Sage object, and can dramatically slow down some algorithms which try things, catch exceptions, and then try other things without looking at the error message.
I think the string formatting here is fine, since it should just be mixing in another simple error message, but it seems wise to leave a warning here.
comment:3 Changed 9 years ago by
In the warning, you should link to the new cholesky
function: :meth:`cholesky`
.
comment:4 Changed 9 years ago by
More seriously, the caching means that this broken, crappy function can infect the new, correct function:
sage: r = matrix(CDF, 2, 2, [ 1, -2*I, 2*I, 0 ]); r [ 1.0 -2.0*I] [ 2.0*I 0.0] sage: r[0,0] = 0 sage: r.cholesky_decomposition() [ 0.0 0.0] [NaN + NaN*I NaN + NaN*I] sage: r.cholesky() [ 0.0 0.0] [NaN + NaN*I NaN + NaN*I]
The documentation makes it clear that cholesky_decomposition
can return bad answers, but the new function shouldn't be infected by that.
Changed 9 years ago by
comment:5 Changed 9 years ago by
- Description modified (diff)
- Reviewers set to Dan Drake
Update patch addresses reviewer comments: cholesky is linked in warning, cache strings are separated, and a doctest for that is added.
Apply new patch on top of old.
comment:6 Changed 9 years ago by
Separating the caches does show us that the new routine is much faster:
sage: A= random_matrix(CDF, 200); M = A*A.conjugate_transpose() sage: %time M.cholesky() CPU times: user 0.01 s, sys: 0.00 s, total: 0.01 s Wall time: 0.03 s 200 x 200 dense matrix over Complex Double Field sage: %time M.cholesky_decomposition() /home/drake/s/sage-5.1.beta0/local/lib/python2.7/site-packages/IPython/Magic.py:1981: DeprecationWarning: (Since Sage Version 5.1) cholesky_decomposition() is deprecated; please use cholesky() instead. out = eval(code,glob) CPU times: user 23.33 s, sys: 0.01 s, total: 23.34 s Wall time: 23.42 s 200 x 200 dense matrix over Complex Double Field
About three orders of magnitude. Nice.
comment:7 Changed 9 years ago by
The second patch is good. Positive review to all.
Apply trac_13045-deprecate-cholesky-decomposition-v1.patch and trac_13045-deprecate-cholesky-decomposition-update.patch
comment:8 Changed 9 years ago by
- Status changed from needs_review to positive_review
comment:9 Changed 9 years ago by
- Merged in set to sage-5.1.beta5
- Resolution set to fixed
- Status changed from positive_review to closed
Importing the deprecation machinery seemed to set off some Python deprecation warnings about messages on exceptions, so those have been modernized.