Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade unitary check for RDF/CDF matrices #11306

Closed
rbeezer mannequin opened this issue May 6, 2011 · 14 comments
Closed

Upgrade unitary check for RDF/CDF matrices #11306

rbeezer mannequin opened this issue May 6, 2011 · 14 comments

Comments

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented May 6, 2011

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. Schur matrix decomposition over RDF/CDF #11027
  2. Checks for Hermitian matrices #10848
  3. Temporarily disable failing SVD doctest #11277

Apply:

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

Depends on #11027
Depends on #10848
Depends on #11277

CC: @jasongrout

Component: linear algebra

Keywords: days30

Author: Rob Beezer

Reviewer: David Loeffler

Merged: sage-5.0.beta9

Issue created by migration from https://trac.sagemath.org/ticket/11306

@rbeezer rbeezer mannequin added this to the sage-5.0 milestone May 6, 2011
@rbeezer rbeezer mannequin added c: linear algebra labels May 6, 2011
@rbeezer rbeezer mannequin assigned jasongrout and williamstein May 6, 2011
@rbeezer

This comment has been minimized.

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 6, 2011

comment:1

Attachment: trac_11306-upgrade-unitary-matrix-check.patch.gz

For the patchbot:

Depends on 11027, 10848, 11277

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 6, 2011

Author: Rob Beezer

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented May 6, 2011

Dependencies: 11027, 10848, 11277

@rbeezer rbeezer mannequin added the s: needs review label May 6, 2011
@saliola
Copy link

saliola commented May 8, 2011

Changed keywords from none to days30

@fchapoton
Copy link
Contributor

Changed dependencies from 11027, 10848, 11277 to #11027, #10848, #11277

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

comment:5

Looks good and passes doctests

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

Reviewer: David Loeffler

@jdemeyer
Copy link

comment:6

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.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 14, 2012

comment:7

Attachment: trac_11306-docfix.patch.gz

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.

@loefflerd loefflerd mannequin added s: needs review and removed s: needs work labels Mar 14, 2012
@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Mar 15, 2012

comment:8

Dear David,

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

Thanks for plowing though the "needs_rewview" backlog.

Rob

@rbeezer

This comment has been minimized.

@rbeezer
Copy link
Mannequin Author

rbeezer mannequin commented Mar 15, 2012

comment:10

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.

@jdemeyer
Copy link

Merged: sage-5.0.beta9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants