Opened 10 years ago

Closed 9 years ago

#11306 closed enhancement (fixed)

Upgrade unitary check for RDF/CDF matrices

Reported by: rbeezer Owned by: jason, was
Priority: minor Milestone: sage-5.0
Component: linear algebra Keywords: days30
Cc: jason Merged in: sage-5.0.beta9
Authors: Rob Beezer Reviewers: David Loeffler
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11027, #10848, #11277 Stopgaps:

Status badges

Description (last modified by rbeezer)

This is an upgrade of the is_unitary() method, based on experience building is_normal(), is_hermitian(). (#10848, #11104)

One test is discovering a bug elsewhere (#11248), so needs to be adjusted slightly to preserve that discovery, though at this writing, the test is disabled (#11277).

(a) Adds a "orthonormal" variant, which is now the default, based on the Schur decomposition, an idea used in the other two methods, but not considered here previously.

(b) Makes the existing naive algorithm a bit more efficent by using the break command twice.

(c) Fixes an ommission in the naive algorithm where the loop on i previously did not start at zero.

(d) Upgraded error-checking on tolerance parameter.

(e) Docstring upgraded to reflect changes above.

Depends on:

  1. #11027
  2. #10848
  3. #11277

Apply:

  1. trac_11306-upgrade-unitary-matrix-check.patch
  2. trac_11306-docfix.patch

Attachments (2)

trac_11306-upgrade-unitary-matrix-check.patch (7.3 KB) - added by rbeezer 10 years ago.
trac_11306-docfix.patch (671 bytes) - added by davidloeffler 9 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 years ago by rbeezer

  • Authors set to Rob Beezer
  • Dependencies set to 11027, 10848, 11277
  • Description modified (diff)
  • Status changed from new to needs_review

For the patchbot:

Depends on 11027, 10848, 11277

comment:2 Changed 10 years ago by saliola

  • Keywords days30 added

comment:3 Changed 10 years ago by chapoton

  • Dependencies changed from 11027, 10848, 11277 to #11027, #10848, #11277

comment:4 Changed 9 years ago by jason

  • Cc jason added

comment:5 Changed 9 years ago by davidloeffler

  • Reviewers set to David Loeffler
  • Status changed from needs_review to positive_review

Looks good and passes doctests

comment:6 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_work

There is some misformatting of the documentation (check http://sagemath.org/doc/developer/conventions.html#documentation-strings for a template):

dochtml.log:docstring of sage.matrix.matrix_double_dense.Matrix_double_dense.is_normal:29: WARNING: Literal block expected; none found.
dochtml.log:docstring of sage.matrix.matrix_double_dense.Matrix_double_dense.is_normal:46: WARNING: Literal block expected; none found.
dochtml.log:docstring of sage.matrix.matrix_double_dense.Matrix_double_dense.is_unitary:32: WARNING: Literal block expected; none found.

Changed 9 years ago by davidloeffler

comment:7 Changed 9 years ago by davidloeffler

  • Status changed from needs_work to needs_review

Two of these aren't related to this ticket (I guess you were testing this and #11104 at the same time). The other one is fixed by the single-character patch above.

comment:8 Changed 9 years ago by rbeezer

Dear David,

Gotta love those one-character patches. I'll get this reviewed as well.

Thanks for plowing though the "needs_rewview" backlog.

Rob

comment:9 Changed 9 years ago by rbeezer

  • Description modified (diff)

comment:10 Changed 9 years ago by rbeezer

  • Status changed from needs_review to positive_review

Positive review. I'm inclined to just leave the author/reviewer fields as-is, but if David wants to double them up, that's fine too.

Thanks for the review, David.

comment:11 Changed 9 years ago by jdemeyer

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