#20439 closed defect (fixed)
eigenmatrix_right gives the conjugate of what it should
Reported by:  tmonteil  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  linear algebra  Keywords:  sd90 
Cc:  Merged in:  
Authors:  Alina Bucur, Renata Paramastri  Reviewers:  Kiran Kedlaya, Rebecca Lauren Miller, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  a70e0db (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
Here is the example reported at http://ask.sagemath.org/question/33084/bugineigenmatrixcommand/
sage: A = matrix(CDF, [[2.53634347567, 2.04801738686, 0.0, 62.166145304], [ 0.7, 0.6, 0.0, 0.0], [0.547271128842, 0.0, 0.3015, 21.7532081652], [0.0, 0.0, 0.3, 0.4]]) sage: D, P = A.eigenmatrix_right()
According to the documentation, D,P
should satisfy A*P == P*D
, however:
sage: (A*P  P*D).norm() 7.454841195108627
The conjugate of P
seems to be the correct answer though:
sage: P = P.conjugate() sage: (A*P  P*D).norm() 6.716506829378007e15
The same issue holds for eigenmatrix_left
sage: sage: D, P = A.eigenmatrix_left() sage: (P*A  D*P).norm() 8.009616737465649 sage: P = P.conjugate() sage: (P*A  D*P).norm() 8.481237941055673e15
Change History (20)
comment:1 Changed 7 years ago by
Description:  modified (diff) 

comment:3 Changed 5 years ago by
Keywords:  sd90 added 

comment:4 Changed 5 years ago by
Branch:  → u/rparamastri/eigenmatrix_right_gives_the_conjugate_of_what_it_should 

comment:5 Changed 5 years ago by
Authors:  → Renata Paramastri 

Commit:  → cc7c2caef70be32f3b1e73830304c5c05b010932 
Status:  new → needs_review 
Conjugated the eigenvectors returned by scipy.linalg.eig
as suggested here.
comment:6 Changed 5 years ago by
Status:  needs_review → needs_work 

All tests passed on k8s, so it looks like there were no unexpected side effects. (This is a bit surprising to me, but never mind.) I do have some comments on the doctests, though.
It is a Sage coding convention that lines should be no longer than 79 characters; anything longer should be split into multiple lines. In matrix2.pyx
, the new doctest in eigenmatrix_left
has a line that violates this convention.
The new doctests should reference this ticket. Sample text to illustrate the formatting:
This test shows that :trac:`20439` has been resolved:: ...
I would add a new doctest to left_eigenvectors
where the substantive change was made, showing the effect of the change and again referencing this ticket. Repurposing the example from the top of the ticket would suffice.
comment:7 Changed 5 years ago by
There should also be a blank line before and after examples, but not after the last example. Also I would reformat the examples so the lines are shorter.
comment:8 Changed 5 years ago by
To elaborate a bit more on the character limit @kedlaya is referring to, the usual way to split long lines is to do:
sage: A = matrix(CDF, [[2.53634347567, 2.04801738686, 0.0, 62.166145304], ....: [0.7, 0.6, 0.0, 0.0], ....: [0.547271128842, 0.0, 0.3015, 21.7532081652], ....: [0.0, 0.0, 0.3, 0.4]])
In some cases, you might have to go over 79 char/line for readability, but we try to stick to that limit as much as possible.
comment:9 Changed 5 years ago by
Branch:  u/rparamastri/eigenmatrix_right_gives_the_conjugate_of_what_it_should → u/kedlaya/eigenmatrix_right_gives_the_conjugate_of_what_it_should 

comment:10 Changed 5 years ago by
Authors:  Renata Paramastri → Alina Bucur, Renata Paramastri 

Commit:  cc7c2caef70be32f3b1e73830304c5c05b010932 → 74b4876f93f5dbce3fcd7d182d1250bd21c27817 
Status:  needs_work → needs_review 
Alina was having trouble with trac just now, so I've pushed her changes.
New commits:
74b4876  Edited doctests

comment:11 Changed 5 years ago by
Reviewers:  → Travis Scrimshaw, Rebecca Miller 

Added Travis Scrimshaw, Rebecca Miller to reviewers.
comment:12 followup: 15 Changed 5 years ago by
@kedlaya, you should add yourself to the reviewers list too.
I also have a few additional suggestions for the doctests:
 sage: for i in range(A.dimensions()[0]):  ....: ((A  spectrum[i][0]*identity_matrix(A.dimensions()[0]))*Matrix(spectrum[i][1]).transpose()).norm()<10^(2) + sage: all(((A  spectrum[i][0]) * Matrix(spectrum[i][1]).transpose()).norm() < 10^(2) + ....: for i in range(A.nrows())
In fact, all of those <
should have a space around them by PEP8 (it also is much easier to read quickly).
Two more spaces after the ....:
in these:
sage: for i in range(len(spectrum)): ....: spectrum[i][1][0]=matrix(CDF, spectrum[i][1]).echelon_form()[0]
comment:13 Changed 5 years ago by
Commit:  74b4876f93f5dbce3fcd7d182d1250bd21c27817 → 3465c52f1bae1391676f1074bbc570ced47a3bb1 

Branch pushed to git repo; I updated commit sha1. New commits:
3465c52  Edited doctests again

comment:14 Changed 5 years ago by
Reviewers:  Travis Scrimshaw, Rebecca Miller → Kiran Kedlaya, Rebecca Miller, Travis Scrimshaw 

Added myself as reviewer and edited the doctests as proposed.
comment:15 followup: 18 Changed 5 years ago by
Thank you. Almost done.
Replying to tscrim:
In fact, all of those
<
should have a space around them by PEP8 (it also is much easier to read quickly).
You missed a few:
(P*A  D*P).norm()<10^(2) +(P*A  D*P).norm() < 10^(2)
Once that is done, you can set a positive review on my behalf.
comment:16 Changed 5 years ago by
Milestone:  sage7.2 → sage8.1 

comment:17 Changed 5 years ago by
Commit:  3465c52f1bae1391676f1074bbc570ced47a3bb1 → a70e0db8a58042d1562ce616b2cb8407a68f84be 

Branch pushed to git repo; I updated commit sha1. New commits:
a70e0db  Yet more edits to fix doctest formatting

comment:18 Changed 5 years ago by
Status:  needs_review → positive_review 

Replying to tscrim:
Thank you. Almost done.
Replying to tscrim:
In fact, all of those
<
should have a space around them by PEP8 (it also is much easier to read quickly).You missed a few:
(P*A  D*P).norm()<10^(2) +(P*A  D*P).norm() < 10^(2)Once that is done, you can set a positive review on my behalf.
Done and done.
comment:19 Changed 5 years ago by
Branch:  u/kedlaya/eigenmatrix_right_gives_the_conjugate_of_what_it_should → a70e0db8a58042d1562ce616b2cb8407a68f84be 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:20 Changed 5 years ago by
Commit:  a70e0db8a58042d1562ce616b2cb8407a68f84be 

Reviewers:  Kiran Kedlaya, Rebecca Miller, Travis Scrimshaw → Kiran Kedlaya, Rebecca Lauren Miller, Travis Scrimshaw 
It appears that both
eigenmatrix_right
andeigenmatrix_left
are computed usingeigenvectors_left
, which forCDF
is implemented insage/matrix/matrix_double_dense.py
. That in turn is callingscipy.linalg.eig
, whose documentationhttps://docs.scipy.org/doc/scipy/reference/generated/scipy.linalg.eig.html
states that indeed for left eigenvectors, the reported "eigenvalues" are actually the conjugates thereof.
So I would propose changing
eigenvectors_left
to account for this.