Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#11595 closed enhancement (fixed)

Update exact eigenspace routines

Reported by: rbeezer Owned by: jason, was
Priority: minor Milestone: sage-4.7.2
Component: linear algebra Keywords:
Cc: Merged in: sage-4.7.2.alpha3
Authors: Rob Beezer Reviewers: Martin Raum, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by leif)

This patch overhauls the generic code for eigenspaces of exact matrices.

  1. Removed plain eigenspaces(), which was deprecated 3 years ago. This required three replacements by eigenspaces_left() in code outside the sage/matrix directory.
  1. Right eigenspaces were not being cached. This is fixed by explicity performning the caching in the method before working on the transpose.
  1. Warning about inexact rings have been removed and replaced by exceptions. These exceptions, and the documentation, suggest the eigenmatrix routines for RDF/CDF matrices where accuracy and speed should be much better (or code for symbolic matrices).
  1. Documentation has been improved.
  1. Only about 4 very minor changes were required outside of the matrix code to make doctests pass.
  1. If basis matrices of eigenspaces over QQbar are hanging, then #11544 might need to be a dependency. Strictly speaking it is not required to build, but could be needed to pass doctests.
  1. One change in behavior - the new format command, and its default value of "all". This will cause eigenspaces of matrices over the rationals formed with algebraic numbers from QQbar. This is consistent with how the eigenvalues() method behaves for rational matrices. When it is not possible to compute eigenvalues this way, the failure is graceful and suggests the format='galois' option. This required just one change outside of the eigen-stuff code (eigenspaces of graphs). See discussion at:

http://groups.google.com/group/sage-devel/browse_thread/thread/c67266a5581abff9/

Apply:

  1. trac_11595-exact-eigenspaces.patch
  2. trac_11595-exact-eigenspaces-format-v1.patch
  3. trac_11595-fix_noisy_zero_doctest_errors.reviewer.patch

Attachments (4)

trac_11595-exact-eigenspaces.patch (32.9 KB) - added by rbeezer 10 years ago.
trac_11595-exact-eigenspaces-format-draft.patch (4.6 KB) - added by rbeezer 10 years ago.
Apply on top of main patch (this is a draft)
trac_11595-exact-eigenspaces-format-v1.patch (11.9 KB) - added by rbeezer 10 years ago.
trac_11595-fix_noisy_zero_doctest_errors.reviewer.patch (1.9 KB) - added by leif 10 years ago.
Reviewer patch. Fixes doctest errors due to noisy zeroes in eigenvalues. Apply on top of other patches.

Download all attachments as: .zip

Change History (17)

Changed 10 years ago by rbeezer

comment:1 Changed 10 years ago by rbeezer

  • Authors set to Rob Beezer
  • Description modified (diff)

comment:2 Changed 10 years ago by rbeezer

  • Description modified (diff)

comment:3 Changed 10 years ago by rbeezer

  • Status changed from new to needs_review

comment:4 follow-up: Changed 10 years ago by mraum

  • Status changed from needs_review to needs_info

Hi Robert, I finally started looking at this patch, which clearly is the most controversial. I read the discussion on sage-devel and I am not sure at all whether there is a real consensus. I know consistency is a strong argument, but I just feel bad with having a standard argument that fails for all but one ring. That won't be beneficial to neither students nor "us", the researchers. What about a standard format = None, that if the base ring is QQ leads to 'all' and, otherwise, to 'galois'. If, in the documentation, you put a warning that for rings different from QQ this might change in the future (in favour of 'all') than any programmer is informed.

comment:5 in reply to: ↑ 4 Changed 10 years ago by rbeezer

Replying to mraum:

Hi Robert,

I read the discussion on sage-devel

Thanks - that is a lot of work all by itself. ;-) I just re-read it myself. You are right that it is the most controversial. Interestingly, consistency was in some measure my goal.

What about a standard format = None, that if the base ring is QQ leads to 'all' and, otherwise, to 'galois'. If, in the documentation, you put a warning that for rings different from QQ this might change in the future (in favour of 'all') than any programmer is informed.

That sounds fine to me. I'll see how it goes coding it up (should be easy) and post once completed.

Rob

Changed 10 years ago by rbeezer

Apply on top of main patch (this is a draft)

comment:6 Changed 10 years ago by rbeezer

I had to straighten out the format before checking to see if the results are cached. So instead of doing it twice, I moved it to a helper method, and then moved error checking there.

Can you look and see if this is the behavior you are suggesting? If it looks OK, I'll add documentation, etc to get it up to standards. Right now it passses all tests on sage/matrix/matrix2.pyx, except for two obvious changes in the error message for the format keyword.

See sage-devel for a post about another problem with all this...

comment:7 Changed 10 years ago by mraum

That quiet exactly what I thought of. I still haven't tested this, but will do this tomorrow. I don't expect to find any problems. Please go ahead and doctest/document the additional changes.

comment:8 follow-up: Changed 10 years ago by mraum

There is one issue that I found, checking the tests. The lines with

- ``format`` - default: 'all' 
  - 'all' - attempts to create every eigenspace.  This will 

lead to indention errors when building the documentation. Please consider this, when updating the documentation.

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

Replying to mraum:

Please consider this, when updating the documentation.

Yes, certainly. Thanks.

Off to Bug Days 32 in just a bit, and will try to wrap this up soon. I have also looked at the "properties" patch and will try to get to that as well. And there is an RDF/CDF patch I know I need to follow up one (some comments about caching).

Am I missing anything else? I know you touched a lot of tickets while Trac was not sending out email.

Thanks, Rob

comment:10 Changed 10 years ago by rbeezer

  • Description modified (diff)
  • Status changed from needs_info to needs_review

"v1 format" patch is ready to go - documented and tested. Apply on top of the main patch.

comment:11 Changed 10 years ago by mraum

  • Reviewers set to Martin Raum
  • Status changed from needs_review to positive_review

Great! Everything works fine.

comment:12 Changed 10 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

Changed 10 years ago by leif

Reviewer patch. Fixes doctest errors due to noisy zeroes in eigenvalues. Apply on top of other patches.

comment:13 Changed 10 years ago by leif

  • Description modified (diff)
  • Reviewers changed from Martin Raum to Martin Raum, Leif Leonhardy

Reviewer patch is up, fixing two doctest errors in sage/matrix/matrix2.pyx.

Note: See TracTickets for help on using tickets.