search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 481 forks source link

Upgrade unitary check for RDF/CDF matrices #11306

Closed 1d7ec08f-60ae-4512-91a6-8324c06eab9f closed 12 years ago

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 13 years ago

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. 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

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 13 years ago

Description changed:

--- 
+++ 
@@ -17,3 +17,7 @@
 2. #10848
 3. #11277

+**Apply:**
+1. [attachment: trac_11306-upgrade-unitary-matrix-check.patch](https://github.com/sagemath/sage-prod/files/10652803/trac_11306-upgrade-unitary-matrix-check.patch.gz)
+
+
1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 13 years ago
comment:1

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

For the patchbot:

Depends on 11027, 10848, 11277

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 13 years ago

Author: Rob Beezer

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 13 years ago

Dependencies: 11027, 10848, 11277

saliola commented 13 years ago

Changed keywords from none to days30

fchapoton commented 13 years ago

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

11d1fc49-71a1-44e1-869f-76be013245a0 commented 12 years ago
comment:5

Looks good and passes doctests

11d1fc49-71a1-44e1-869f-76be013245a0 commented 12 years ago

Reviewer: David Loeffler

jdemeyer commented 12 years ago
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.
11d1fc49-71a1-44e1-869f-76be013245a0 commented 12 years ago
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.

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 12 years ago
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

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 12 years ago

Description changed:

--- 
+++ 
@@ -19,5 +19,6 @@

 **Apply:**
 1. [attachment: trac_11306-upgrade-unitary-matrix-check.patch](https://github.com/sagemath/sage-prod/files/10652803/trac_11306-upgrade-unitary-matrix-check.patch.gz)
+2. [attachment: trac_11306-docfix.patch](https://github.com/sagemath/sage-prod/files/10652804/trac_11306-docfix.patch.gz)
1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 12 years ago
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 commented 12 years ago

Merged: sage-5.0.beta9