Opened 14 years ago
Closed 12 years ago
#4756 closed defect (fixed)
eigenmatrix_right totally broken
Reported by: | William Stein | Owned by: | William Stein |
---|---|---|---|
Priority: | major | Milestone: | sage-4.4 |
Component: | linear algebra | Keywords: | |
Cc: | Jason Grout | Merged in: | sage-4.4.alpha0 |
Authors: | Rob Beezer | Reviewers: | Alex Ghitza |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
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 (4)
Change History (17)
comment:1 Changed 14 years ago by
Cc: | Jason Grout added |
---|
comment:2 Changed 13 years ago by
Report Upstream: | → N/A |
---|---|
Status: | new → needs_work |
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 13 years ago by
Attachment: | trac_4756_eigenmatrices_double.patch added |
---|
Changed 13 years ago by
Attachment: | trac_4756-double-eigen.patch added |
---|
Self-contained patch, apply only this
comment:3 Changed 13 years ago by
Authors: | → 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.
comment:4 Changed 13 years ago by
Status: | needs_work → needs_review |
---|
comment:5 Changed 13 years ago by
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()
comment:6 Changed 13 years ago by
Status: | needs_review → needs_work |
---|
comment:7 Changed 13 years ago by
Status: | needs_work → 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 13 years ago by
Attachment: | trac_4756-double-eigen.2.patch added |
---|
Self-contained patch, apply only this.
comment:8 Changed 13 years ago by
Status: | needs_review → 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.
comment:9 Changed 13 years ago by
Status: | needs_work → needs_review |
---|
dot-3 patch has better doctests in it, totally avoiding "zero" eigenvalues and "zero" entries of eigenvectors. Apply just this one patch.
comment:10 follow-up: 11 Changed 13 years ago by
Status: | needs_review → needs_info |
---|
Has this been checked on Solaris?
There's general information about building on Solaris at
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
comment:11 Changed 13 years ago by
Status: | needs_info → 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
comment:12 Changed 12 years ago by
Reviewers: | → Alex Ghitza |
---|---|
Status: | needs_review → positive_review |
Looks good to me.
comment:13 Changed 12 years ago by
Merged in: | → sage-4.4.alpha0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Merged "trac_4756-double-eigen.3.patch" in 4.4.alpha0
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 :-)