Opened 11 years ago
Closed 11 years ago
#11897 closed defect (fixed)
Fix eigenmatrix doctest to work across all platforms
Reported by: | rbeezer | Owned by: | jason, was |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.7.2 |
Component: | linear algebra | Keywords: | |
Cc: | kcrisman, jdemeyer, leif | Merged in: | sage-4.7.2.alpha4 |
Authors: | Rob Beezer | Reviewers: | Karl-Dieter Crisman, Leif Leonhardy |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Doctest for RDF matrix fails on some platforms by returning the negatives of the more commonly returned eigenvectors.
See sage-release discussion: http://groups.google.com/group/sage-release/msg/e885e53316c6880f
Apply
- trac_11897-doctest-RDF-eigenmatrix.rebased.patch
- trac_11897-fix_noisy_zeroes_in_eigenvalues.reviewer.patch
to the Sage library.
Attachments (3)
Change History (22)
Changed 11 years ago by
comment:1 follow-up: ↓ 2 Changed 11 years ago by
- Status changed from new to needs_review
comment:2 in reply to: ↑ 1 ; follow-up: ↓ 4 Changed 11 years ago by
- Status changed from needs_review to needs_work
Replying to rbeezer:
This was built on an alpha3 prerelease, I trust it will be OK on a real alpha3 (which I am about to build right now).
Two hunks do not apply because of (the late) ticket:11595:trac_11595-fix_noisy_zero_doctest_errors.reviewer.patch.
Otherwise looks fine, but perhaps Karl-Dieter should rerun the tests on his famous favorite machine.
comment:3 follow-up: ↓ 5 Changed 11 years ago by
Do you also open a follup-up ticket to #7852?
ticket:7852:trac_7852-adjust_noisy_zero_term_threshold_for_polys.reviewer.patch wasn't enough for his machine. (I later increased epsilon from 1.0e-15 to 2.5e-15 in one example to make bsd.math
happy, but he gets 2.6645352591e-15.)
comment:4 in reply to: ↑ 2 Changed 11 years ago by
Two hunks do not apply because of (the late) ticket:11595:trac_11595-fix_noisy_zero_doctest_errors.reviewer.patch.
Correct. Let me know when you have a new one. :(
Otherwise looks fine, but perhaps Karl-Dieter should rerun the tests on his famous favorite machine.
Now, now! No disparaging remarks. At least I'm not using Windows or FreeBSD! Those pose more drastic problems :)
comment:5 in reply to: ↑ 3 ; follow-up: ↓ 16 Changed 11 years ago by
Replying to leif:
Do you also open a follup-up ticket to #7852?
No, did not want to venture into polynomials - just took responsibility for matrices.
Forgot to actually start a build last night, so it will be maybe 12 hours at the soonest before I can build a proper patch (long story). But will do.
Rob
comment:6 follow-up: ↓ 7 Changed 11 years ago by
- Description modified (diff)
- Reviewers set to Karl-Dieter Crisman, Leif Leonhardy
- Status changed from needs_work to needs_review
Sorry, I could easily have done that yesterday, but I was too tired to even look at the rejects.
The rebased patch also passes tests and the documentation builds fine, so positive review from my side.
Karl-Dieter (or Dasher / student?), please finalize!
comment:7 in reply to: ↑ 6 Changed 11 years ago by
Karl-Dieter (or Dasher / student?), please finalize!
You don't even want to know how old this computer is, nor how many hands it's passed through on the way to me. But luckily the password is still rms' favorite - carriage return - and its only use is for testing Sage, so that's all I need! I know it's a burden sometimes, but I think that it's picked up a number of (real) bugs over the last couple years as well, vindicating Dave Kirkby's philosophy of "test as widely as possible".
Anyway, I'll get that fired up tomorrow morning when I get in.
Eventually I'll probably convert it (and a couple other similar machines I own) to Linux. Though now that YouTube has the HTML5 option, maybe I can start avoiding Flash and keep them going a few more years... see also TenFourFox. Favorite quote: "A quad 2.5GHz G5 isn't worth using to surf the web? Really? And you guys still support Windows XP?"
comment:8 follow-ups: ↓ 9 ↓ 13 Changed 11 years ago by
- Status changed from needs_review to needs_work
Really sorry, folks.
Dasher-03:~/Desktop/sage-4.7.2.alpha3/devel/sage student$ ../../sage -t -long devel/sage/sage/matrix/matrix2.pyx sage -t -long "devel/sage/sage/matrix/matrix2.pyx" ********************************************************************** File "/Users/student/Desktop/sage-4.7.2.alpha3/devel/sage/sage/matrix/matrix2.pyx", line 5282: sage: evalues = em[0]; evalues Expected: [ 13.3484692... 0 0] [ 0 -1.34846922... 0] [ 0 0 -6.2265089...e-16] Got: [ 13.3484692283 0 0] [ 0 -1.34846922835 0] [ 0 0 -1.35443935375e-15] ********************************************************************** File "/Users/student/Desktop/sage-4.7.2.alpha3/devel/sage/sage/matrix/matrix2.pyx", line 5371: sage: evalues = em[0]; evalues Expected: [ 13.3484692... 0 0] [ 0 -1.34846922... 0] [ 0 0 -8.86256604...e-16] Got: [ 13.3484692283 0 0] [ 0 -1.34846922835 0] [ 0 0 1.74841524385e-16] ********************************************************************** 2 items had failures: 1 of 21 in __main__.example_63 1 of 21 in __main__.example_64 ***Test Failed*** 2 failures. For whitespace errors, see the file /Users/student/.sage//tmp/matrix2_27324.py [262.0 s] ---------------------------------------------------------------------- The following tests failed: sage -t -long "devel/sage/sage/matrix/matrix2.pyx"
We may need to use the new # tol
construction for this one :(
comment:9 in reply to: ↑ 8 Changed 11 years ago by
Replying to kcrisman:
We may need to use the new
# tol
construction for this one :(
Hmmm, would be nicer to expect a true zero (0
) there and once again use .zero_at(2e-15)
(or 2e-16
in one case)...
If you use # tol
, that applies to all entries in the same way, i.e. with the same relative or absolute tolerance, which IMHO wouldn't be appropriate here.
comment:10 Changed 11 years ago by
Nope, .dense_matrix().zero_at(2e-15)
in both cases, and expect 0
in the lower right.
comment:11 follow-up: ↓ 14 Changed 11 years ago by
- Description modified (diff)
Next try.
(I've attached a reviewer patch to be applied on top.)
Hopefully 2e-15
suffices for bsd.math
as well... Apple cp...
comment:12 Changed 11 years ago by
- Status changed from needs_work to needs_review
comment:13 in reply to: ↑ 8 Changed 11 years ago by
Replying to kcrisman:
Really sorry, folks.
No need to apologize - I was sort of afraid this would happen.
I have an alpha3 build now and will get to whatever needs doing (review of the latest patch?) tonight if it is still outstanding.
Rob
comment:14 in reply to: ↑ 11 Changed 11 years ago by
comment:15 follow-up: ↓ 17 Changed 11 years ago by
- Status changed from needs_review to positive_review
I agree that this solution is better than what I suggested. This passes the tests. Positive review?
sage -t -long "devel/sage/sage/matrix/matrix2.pyx" [261.2 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 261.5 seconds
comment:16 in reply to: ↑ 5 Changed 11 years ago by
comment:17 in reply to: ↑ 15 ; follow-up: ↓ 18 Changed 11 years ago by
Replying to kcrisman:
I agree that this solution is better than what I suggested. This passes the tests. Positive review?
Why not... Although Rob could review my reviewer patch.
Anyway, he can set it back to needs work just in case... :)
comment:18 in reply to: ↑ 17 Changed 11 years ago by
Replying to leif:
Why not... Although Rob could review my reviewer patch.
Anyway, he can set it back to needs work just in case... :)
It all looks good to me, so I think we are done with this one. I'll go look at polynomials.
Thanks for the help with this one, Leif, and to KDC and Dasher for pushing us to do better. ;-)
comment:19 Changed 11 years ago by
- Merged in set to sage-4.7.2.alpha4
- Resolution set to fixed
- Status changed from positive_review to closed
Rationale: this was just meant to show how to get results for inexact matrices with real or complex entries. It is in the middle of a docstring for an exact routine. So this preserves the "doc" part and abandons the "test" part.
Rational: doctest has not been abandoned. Due to the need to adjust the sign of the eigenvectors, this is relegated to a test section.
This was built on an alpha3 prerelease, I trust it will be OK on a real alpha3 (which I am about to build right now).