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 rbeezer)

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:

  1. trac_13045-deprecate-cholesky-decomposition-v1.patch
  2. trac_13045-deprecate-cholesky-decomposition-update.patch

Attachments (2)

Change History (11)

comment:1 Changed 9 years ago by rbeezer

  • Authors set to Rob Beezer
  • Cc ddrake added
  • Dependencies set to #12966, #13018, #11274, #13035
  • Description modified (diff)
  • Keywords sd40.5 added
  • Status changed from new to needs_review

Importing the deprecation machinery seemed to set off some Python deprecation warnings about messages on exceptions, so those have been modernized.

comment:2 Changed 9 years ago by ddrake

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 ddrake

In the warning, you should link to the new cholesky function: :meth:`cholesky`.

comment:4 Changed 9 years ago by ddrake

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.

comment:5 Changed 9 years ago by rbeezer

  • 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 ddrake

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 ddrake

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 ddrake

  • Status changed from needs_review to positive_review

comment:9 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.1.beta5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.