#20439 closed defect (fixed)
eigenmatrix_right gives the conjugate of what it should
Reported by: | tmonteil | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.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/bug-in-eigenmatrix-command/
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.716506829378007e-15
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.481237941055673e-15
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 follow-up: 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 follow-up: 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: | sage-7.2 → sage-8.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.