search

sagemath / sage

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

Add check for normal matrices #11104

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

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

An exact method that compares entries of the two products and an RDF/CDF version that has both a naive check like the exact case and a more robust check based on the Schur decomposition.

Depends on #11027, #10848

Apply

  1. attachment: trac_11104-normal-matrices.patch
  2. attachment: trac_11104-docfix.patch

Depends on #11027 Depends on #10848

CC: @jasongrout

Component: linear algebra

Author: Rob Beezer

Reviewer: David Loeffler

Merged: sage-5.0.beta9

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

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

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
 An exact method that compares entries of the two products and an RDF/CDF version that has both a naive check like the exact case and a more robust check based on the Schur decomposition.

 Depends on #11027, #10848
+
+Apply trac_11104-normal-matrices.patch
1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 13 years ago

Author: Rob Beezer

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

Attachment: trac_11104-normal-matrices.patch.gz

fchapoton commented 13 years ago

Dependencies: #11027, #10848

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

Reviewer: David Loeffler

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

Looks good, and all doctests pass

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

Attachment: trac_11104-docfix.patch.gz

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

Jeroen pointed out at #11306 two warnings building the documentation that actually come from this ticket. Here's a patch that fixes them.

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

David - you are far too fast for me. I was going to try to clean up my mess. So I'll review the fix instead.

On sage-5.0.beta5 these patches both apply and the tests in sage/matrix all pass. Documentation builds without errors and the is_normal() section of matrix_double_dense.html looks fine.

I'll run full tests in a minute and then flip to positive review.

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

Description changed:

--- 
+++ 
@@ -2,4 +2,6 @@

 Depends on #11027, #10848

-Apply trac_11104-normal-matrices.patch
+**Apply**
+1.  [attachment: trac_11104-normal-matrices.patch](https://github.com/sagemath/sage-prod/files/10652535/trac_11104-normal-matrices.patch.gz)
+2.  [attachment: trac_11104-docfix.patch](https://github.com/sagemath/sage-prod/files/10652536/trac_11104-docfix.patch.gz)
1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 12 years ago
comment:9

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

jhpalmieri commented 12 years ago
comment:11

Using the sage-5.0.beta10-gcc tarball from #12369, I get a doctest failure on OS X Lion:

sage -t  --long -force_lib devel/sage/sage/matrix/matrix_double_dense.pyx
**********************************************************************
File "/Users/palmieri/Desktop/Sage_stuff/sage_builds/GCC-no-check/sage-5.0.beta10-gcc/devel/sage-main/sage/matrix/matrix_double_dense.pyx", line 2843:
    sage: B.is_normal(algorithm='naive', tol=1.0e-34)
Expected:
    False
Got:
    True
**********************************************************************

If I run this by hand, then even

    sage: B.is_normal(algorithm='naive', tol=1.0e-300)

returns True. The matrix B:

sage: B
[                                    1.0 3.46944695195e-16 + 3.88578058619e-16*I 2.22044604925e-16 + 2.77555756156e-17*I]
[3.46944695195e-16 - 3.88578058619e-16*I                                     1.0 6.93889390391e-17 - 1.11022302463e-16*I]
[2.22044604925e-16 - 2.77555756156e-17*I 6.93889390391e-17 + 1.11022302463e-16*I                                     1.0]

If I run B.is_normal(algorithm='orthonormal', tol=1.0e-16), I get False. With tol=1.0e-15, I get True.

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

Replying to @jhpalmieri:

Thanks, John. Maybe this test is just too clever. ;-) It is trying to get a "wrong" answer by using a tolerance that is too strict. With what I know now about variability across OS'es, I would probably not attempt such a thing again. We could,

(a) Back it all out and make a fix, or

(b) Make a corrective ticket.

A fix could be

(i) Ditch the test, or

(ii) Keep it in spirit, but switch to the orthonormal version.

Any thoughts on the best way to proceed?

Rob

jhpalmieri commented 12 years ago
comment:13

Hi Rob,

I prefer options (b) (definitely: no need to back this out for a failure on one platform) and (i) (not as definitely: do you think having a test like this is important? I can't tell...). If you want option (ii), it might be annoying to make sure it passes on all of the different supported architectures.

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

Replying to @jhpalmieri:

Thanks, John.

I like (b)(i) as well. I don't think the test adds much in the way of "software assurance" and was meant to be "educational" for the newbie. So it shouldn't be missed. I'll see if I can get to it tonight.

Rob

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

Replying to @jhpalmieri:

Patch up shortly on #12764.