#11104 closed enhancement (fixed)
Add check for normal matrices
Reported by: | rbeezer | Owned by: | jason, was |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.0 |
Component: | linear algebra | Keywords: | |
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 | Stopgaps: |
Description (last modified by )
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.
Apply
Attachments (2)
Change History (17)
Changed 9 years ago by
comment:1 Changed 9 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
- Dependencies set to #11027, #10848
comment:3 Changed 8 years ago by
- Cc jason added
comment:4 Changed 8 years ago by
- Reviewers set to David Loeffler
- Status changed from needs_review to positive_review
Changed 8 years ago by
comment:5 Changed 8 years ago by
- Status changed from positive_review to needs_work
comment:6 Changed 8 years ago by
- Status changed from needs_work to needs_review
Jeroen pointed out at #11306 two warnings building the documentation that actually come from this ticket. Here's a patch that fixes them.
comment:7 Changed 8 years ago by
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.
comment:8 Changed 8 years ago by
- Description modified (diff)
comment:9 Changed 8 years ago by
- 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:10 Changed 8 years ago by
- Merged in set to sage-5.0.beta9
- Resolution set to fixed
- Status changed from positive_review to closed
comment:11 follow-up: ↓ 12 Changed 8 years ago by
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
.
comment:12 in reply to: ↑ 11 Changed 8 years ago by
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
comment:13 follow-ups: ↓ 14 ↓ 15 Changed 8 years ago by
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.
comment:14 in reply to: ↑ 13 Changed 8 years ago by
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
comment:15 in reply to: ↑ 13 Changed 8 years ago by
Replying to jhpalmieri:
Patch up shortly on #12764.
Looks good, and all doctests pass