eigenmatrix_right gives the conjugate of what it should

[tmonteil]

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

[kedlaya]

It appears that both eigenmatrix_right and eigenmatrix_left are computed using eigenvectors_left, which for CDF is implemented in sage/matrix/matrix_double_dense.py. That in turn is calling scipy.linalg.eig, whose documentation

​https://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.

[rparamastri]

Conjugated the eigenvectors returned by scipy.linalg.eig as suggested ​here.

[kedlaya]

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.

[rlmiller]

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.

[tscrim]

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.

[kedlaya]

Alina was having trouble with trac just now, so I've pushed her changes.

---

New commits:

​74b4876

Edited doctests

[alina]

Added ​Travis Scrimshaw, Rebecca Miller to reviewers.

[tscrim]

@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]

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​3465c52

Edited doctests again

[kedlaya]

Added myself as reviewer and edited the doctests as proposed.

[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.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​a70e0db

Yet more edits to fix doctest formatting

[kedlaya]

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.