Opened 10 years ago

Closed 10 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:

Status badges

Description (last modified by leif)

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

  1. trac_11897-doctest-RDF-eigenmatrix.rebased.patch
  2. trac_11897-fix_noisy_zeroes_in_eigenvalues.reviewer.patch

to the Sage library.

Attachments (3)

trac_11897-doctest-RDF-eigenmatrix.patch (5.2 KB) - added by rbeezer 10 years ago.
trac_11897-doctest-RDF-eigenmatrix.rebased.patch (5.2 KB) - added by leif 10 years ago.
Rob's patch rebased on Sage 4.7.2.alpha3.
trac_11897-fix_noisy_zeroes_in_eigenvalues.reviewer.patch (1.7 KB) - added by leif 10 years ago.
Sage library patch. Apply on top of Rob's (rebased) patch.

Download all attachments as: .zip

Change History (22)

Changed 10 years ago by rbeezer

comment:1 follow-up: Changed 10 years ago by rbeezer

  • Authors set to Rob Beezer
  • Status changed from new to needs_review
  1. Patch marks failing doctests as "not tested"

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.

  1. Patch repeats, and fixes, doctest in the TEST section of the eigenmatrix routines.

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

comment:2 in reply to: ↑ 1 ; follow-up: Changed 10 years ago by leif

  • 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: Changed 10 years ago by leif

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 10 years ago by kcrisman

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: Changed 10 years ago by rbeezer

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

Changed 10 years ago by leif

Rob's patch rebased on Sage 4.7.2.alpha3.

comment:6 follow-up: Changed 10 years ago by leif

  • 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 10 years ago by kcrisman

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: Changed 10 years ago by kcrisman

  • 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 10 years ago by leif

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 10 years ago by leif

Nope, .dense_matrix().zero_at(2e-15) in both cases, and expect 0 in the lower right.

Changed 10 years ago by leif

Sage library patch. Apply on top of Rob's (rebased) patch.

comment:11 follow-up: Changed 10 years ago by leif

  • 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 10 years ago by leif

  • Status changed from needs_work to needs_review

comment:13 in reply to: ↑ 8 Changed 10 years ago by rbeezer

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 10 years ago by leif

Replying to leif:

Hopefully 2e-15 suffices for bsd.math as well...

Surprisingly it does. :)

comment:15 follow-up: Changed 10 years ago by kcrisman

  • 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 10 years ago by leif

Replying to rbeezer:

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.

Patch for that is up on #11901, needing review... :P

comment:17 in reply to: ↑ 15 ; follow-up: Changed 10 years ago by leif

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 10 years ago by rbeezer

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 10 years ago by jdemeyer

  • Merged in set to sage-4.7.2.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.