Ticket #4756 (closed defect: fixed)

Opened 21 months ago

Last modified 5 months ago

eigenmatrix_right totally broken

Reported by: was Owned by: was
Priority: major Milestone: sage-4.4
Component: linear algebra Keywords:
Cc: jason Author(s): Rob Beezer
Report Upstream: N/A Reviewer(s): Alex Ghitza
Merged in: sage-4.4.alpha0 Work issues:

Description

sage: a = matrix(CDF,2,[1,2,4,7])
sage: a.eigenmatrix_right()
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)

/Users/wstein/sage/build/sage-3.2.2.alpha0/<ipython console> in <module>()

/Users/wstein/sage/build/sage-3.2.2.alpha0/local/lib/python2.5/site-packages/sage/matrix/matrix2.so in sage.matrix.matrix2.Matrix.eigenmatrix_right (sage/matrix/matrix2.c:18170)()

/Users/wstein/sage/build/sage-3.2.2.alpha0/local/lib/python2.5/site-packages/sage/matrix/matrix2.so in sage.matrix.matrix2.Matrix.eigenmatrix_left (sage/matrix/matrix2.c:17965)()

IndexError: list index out of range

Attachments

trac_4756_eigenmatrices_double.patch Download (7.6 KB) - added by rbeezer 8 months ago.
trac_4756-double-eigen.patch Download (13.6 KB) - added by rbeezer 7 months ago.
Self-contained patch, apply only this
trac_4756-double-eigen.2.patch Download (13.6 KB) - added by rbeezer 7 months ago.
Self-contained patch, apply only this.
trac_4756-double-eigen.3.patch Download (14.8 KB) - added by rbeezer 7 months ago.
Self conatined

Change History

  Changed 21 months ago by mhansen

  • cc jason added

This is because the eigenvectors_left and eigenvectors_right for CDF and RDF matrices returns the data in a way that is totally incompatible with what the parent is expecting.

I can make a patch for this once I have a 3.2.2alpha1 build. Otherwise, if Jason would like to do this, I wouldn't object :-)

  Changed 8 months ago by rbeezer

  • status changed from new to needs_work
  • upstream set to N/A

Patch causes left_eigenvectors() and right_eigenvectors() for CDF and RDF to return eigenvalues and eigenvectors in the same format as for exact matrices. Then eigenmatrices for these matrices behave as they should. No attempt is made to identify eigenvalues with multiplicity greater than one.

Patch is failing doctests in the modular form code right now, so will need some more work.

Changed 8 months ago by rbeezer

Changed 7 months ago by rbeezer

Self-contained patch, apply only this

  Changed 7 months ago by rbeezer

  • author set to Rob Beezer

This patch fixes the bug and generally brings double-precision eigenvectors up-to-date.

left_eigenvectors and right_eigenvectors now return their results in the same format as for exact matrices, so routines like eigenmatrix_right can digest the results properly.

Consequently, eigenspaces_left was adjusted for this new format. There was no eigenspaces_right, now there is.

The changed code in modular/modform/numerical.py simply adjusts for the new format to allow doctests to pass and is probably sub-optimal. There is a bug nearby, so this will be addressed on #8018.

Doctests: I had some strange behavior for zero eigenvalues (ie very, very small eigenvalues), so the doctests avoid these. For these eigenvalues, the tests would fail on a first run, but would pass on all subsequent runs (or vice-versa). This was observed on my own machine and boxen.math.washington.edu. So I've avoided these eigenvalues by selecting certain ones in the doctests.

Documentation: documentation was tested for these four functions via notebook introspection. The rest of the file needs attention before it can go into the documentation, see #8046.

  Changed 7 months ago by rbeezer

  • status changed from needs_work to needs_review

  Changed 7 months ago by jason

Great job!

I think these changes would make the code easier to read:

 	662	        for k in range(len(spectrum)): 
 	663	            evalue = spectrum[k][0] 
 	664	            evector = spectrum[k][1][0] 
 	665	            pairs.append((evalue, evector.parent().span_of_basis([evector],check=False))) 

changed to

for eval,evectors in spectrum:
    evec = evectors[0]
    evector = evec.parent().span_of_basis([evec],check=False)
    pairs.append((evalue, evector))

(similarly on lines 722-725)

Also:

B = matrix(CDF, [spectrum[i][1][0] for i in range(len(spectrum))]).transpose() 

changed to

B = matrix(CDF, [evecs[0] for _,evecs in spectrum]).transpose() 

  Changed 7 months ago by jason

  • status changed from needs_review to needs_work

  Changed 7 months ago by rbeezer

  • status changed from needs_work to needs_review

Jason,

Thanks for helping me be more Pythonic.

The bit in the modular form module is going to be replaced in #8018 by totally different code, so I didn't change that. First suggestion (in spirit) has been incorporated in updated patch.

Rob

Changed 7 months ago by rbeezer

Self-contained patch, apply only this.

  Changed 7 months ago by rbeezer

  • status changed from needs_review to needs_work

I've got some doctests failing on another machine due to "zero" eigenvalues, so I'm going to rework some of the doctests.

Changed 7 months ago by rbeezer

Self conatined

  Changed 7 months ago by rbeezer

  • status changed from needs_work to needs_review

dot-3 patch has better doctests in it, totally avoiding "zero" eigenvalues and "zero" entries of eigenvectors. Apply just this one patch.

follow-up: ↓ 11   Changed 6 months ago by drkirkby

  • status changed from needs_review to needs_info

Has this been checked on Solaris?

There's general information about building on Solaris at

 http://wiki.sagemath.org/solaris

Information specifically for 't2' at

 http://wiki.sagemath.org/devel/Building-Sage-on-the-T5240-t2

Both the source (4.3.0.1 is the latest to build on Solaris) and a binary which will run on any SPARC can be found at http://www.sagemath.org/download-source.html

Dave

in reply to: ↑ 10   Changed 6 months ago by rbeezer

  • status changed from needs_info to needs_review

Replying to drkirkby:

Has this been checked on Solaris?

Hi Dave,

Not that I know of. I develop with KUbuntu.

Maybe a reviewer with Solaris experience can put it through its paces. Totally Python, so I'd guess numerical issues might be the only distinction.

Thanks, Rob

  Changed 5 months ago by AlexGhitza

  • status changed from needs_review to positive_review
  • reviewer set to Alex Ghitza

Looks good to me.

  Changed 5 months ago by jhpalmieri

  • status changed from positive_review to closed
  • resolution set to fixed
  • merged set to sage-4.4.alpha0

Merged "trac_4756-double-eigen.3.patch" in 4.4.alpha0

Note: See TracTickets for help on using tickets.