#30393 closed enhancement (fixed)
eigenvalues and eigenvectors using arb
Reported by:  mmezzarobba  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  linear algebra  Keywords:  
Cc:  Merged in:  
Authors:  Marc Mezzarobba  Reviewers:  Sébastien Labbé 
Report Upstream:  N/A  Work issues:  
Branch:  b792cbd (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
The wrappers are marked as experimental because the corresponding arb functions are.
Change History (32)
comment:1 Changed 2 years ago by
Branch:  → u/mmezzarobba/acb_eig 

Commit:  → 1d42e2b2b1142a1ec7ea2cc8adb282521c94120e 
Description:  modified (diff) 
Status:  new → needs_review 
comment:2 followup: 4 Changed 2 years ago by
Note that the signature of that method is changing in #29243. Maybe better to follow that?
comment:3 Changed 2 years ago by
Commit:  1d42e2b2b1142a1ec7ea2cc8adb282521c94120e → e89ac6348761059cf038dbfdcc4436b987b633de 

Branch pushed to git repo; I updated commit sha1. New commits:
e89ac63  #30393 match the new interface introduced in #29243

comment:4 Changed 2 years ago by
comment:5 Changed 2 years ago by
Commit:  e89ac6348761059cf038dbfdcc4436b987b633de → 56f22b122b2df7ce7757b01ee4ef13ac1922066a 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
56f22b1  #30393 match the new interface introduced in #29243

comment:6 followup: 8 Changed 2 years ago by
Few comments:
"unable to certify the eigenvalues of this interval matrix"
, I would avoid the usage of the wordinterval
which reminds of theCIF
since the matrix belongs rather to theCBF
. the
other
input should be documented as related to the generalized eigenvalue problem in the input block and/or in the raiseNotImplementedError
.
What should happen when the parent is RealBallField
?
sage: M = matrix(3, [1, 1, 1, 1, 0, 0, 0, 1, 0]) sage: MRBF = M.change_ring(RBF) sage: MRBF.eigenvalues() KeyError: '_PolynomialRing_singular_repr__singular' ... During handling of the above exception, another exception occurred: ... AttributeError: 'PolynomialRing_field_with_category' object has no attribute '_PolynomialRing_singular_repr__singular' ... During handling of the above exception, another exception occurred: ... TypeError: no conversion of this ring to a Singular ring defined ... During handling of the above exception, another exception occurred: ... NotImplementedError:
Can this benefits from your current branch?
comment:7 Changed 2 years ago by
Commit:  56f22b122b2df7ce7757b01ee4ef13ac1922066a → 7f1b45ef37d635b6017566f90b28c94a9517d231 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7f1b45e  #30393 match the new interface introduced in #29243

comment:8 followup: 10 Changed 2 years ago by
Replying to slabbe:
Few comments:
"unable to certify the eigenvalues of this interval matrix"
, I would avoid the usage of the wordinterval
which reminds of theCIF
since the matrix belongs rather to theCBF
. the
other
input should be documented as related to the generalized eigenvalue problem in the input block and/or in the raiseNotImplementedError
.
Done. (Though the second point is bugware. What we need IMHO is for each method's documentation to include or link to that of the methods it overrides, and to change the way we write docstrings to take advantage of that. It doesn't make any sense to repeat the same information over and over with no automatic way of keeping the various places in sync.)
What should happen when the parent is
RealBallField
?
[...]
Can this benefits from your current branch?
In principle yes (and matrices over RR and CC can benefit too), but at the moment we don't even have a dedicated class for matrices over RBF. I think we should keep that for another ticket, and probably wait until the methods are no longer marked experimental.
comment:9 Changed 2 years ago by
Commit:  7f1b45ef37d635b6017566f90b28c94a9517d231 → 5f6fb40362912ff5870e3e5f76abbd902243ee17 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5f6fb40  #30393 wrap arb's experimental eigensolver

comment:10 followup: 14 Changed 2 years ago by
Replying to mmezzarobba:
Replying to slabbe: Done.
Thanks.
(Though the second point is bugware. What we need IMHO is for each method's documentation to include or link to that of the methods it overrides, and to change the way we write docstrings to take advantage of that. It doesn't make any sense to repeat the same information over and over with no automatic way of keeping the various places in sync.)
I agree with your point (out of sync). But, I do hate the documentation of G.plot(**kwds)
which does not say which options are available.
That being said, I think my point in comment 2 was mostly to avoid using the extend
option as a positional argument like:
sage: M = M.change_ring(CBF) sage: M.eigenvalues(True)
since it will not be the 1st argument anymore after #29243. If you can force the extend
to be a keyword argument like def eigenvalues(self, *, extend=False)
, then for me it is okay, even without the other
argument. Do you mind to remove the other
argument? Sorry for my slow convergence on clarifying my point of view.
I think we should keep that for another ticket, and probably wait until the methods are no longer marked experimental.
I agree.
comment:11 followup: 15 Changed 2 years ago by
I get an error while building the documentation:
[dochtml] OSError: docstring of sage.matrix.matrix_complex_ball_dense.Matrix_complex_ball_dense.trace:11: WARNING: Duplicate explicit target name: "arb documentation".
In fact it seems to be triplicate:
+ See the `Arb documentation <http://arblib.org/acb_mat.html#c.acb_mat_eig_multiple>`_ + for more information. ... + See the `Arb documentation <http://arblib.org/acb_mat.html#c.acb_mat_approx_eig_qr>`_ + for more information. ... + See the `Arb documentation <http://arblib.org/acb_mat.html#c.acb_mat_eig_simple>`_ + for more information.
comment:12 Changed 2 years ago by
Status:  needs_review → needs_work 

comment:13 Changed 2 years ago by
Commit:  5f6fb40362912ff5870e3e5f76abbd902243ee17 → edd1ae05e84205894df598c2b77ad45b9a2fdd58 

Branch pushed to git repo; I updated commit sha1. New commits:
edd1ae0  #30393 work around sphinx issue

comment:14 Changed 2 years ago by
Replying to slabbe:
If you can force the
extend
to be a keyword argument likedef eigenvalues(self, *, extend=False)
, then for me it is okay, even without theother
argument. Do you mind to remove theother
argument?
It may still be slightly better to have it there, to be compatible with the generic method in case someone explicitly passes a second argument equal to None
.
comment:15 Changed 2 years ago by
Status:  needs_work → needs_review 

Replying to slabbe:
I get an error while building the documentation:
[dochtml] OSError: docstring of sage.matrix.matrix_complex_ball_dense.Matrix_complex_ball_dense.trace:11: WARNING: Duplicate explicit target name: "arb documentation".
I don't understand what is going on (why should two different inline links necessarily have distinct link texts?!), but let's see if the commit I just pushed solves the issue.
comment:16 Changed 2 years ago by
Can you do the following change (x3) ?
 See the <http://arblib.org/acb_mat.html#c.acb_mat_eig_multiple for + See http://arblib.org/acb_mat.html#c.acb_mat_eig_multiple for
comment:17 Changed 2 years ago by
also:
 http://arblib.org/acb_mat.html#c.acb_mat_approx_eig_qr_ + http://arblib.org/acb_mat.html#c.acb_mat_approx_eig_qr
comment:18 Changed 2 years ago by
Status:  needs_review → needs_work 

comment:19 Changed 2 years ago by
Reviewers:  → Sébastien Labbé 

comment:20 Changed 2 years ago by
Commit:  edd1ae05e84205894df598c2b77ad45b9a2fdd58 → a39c8c65063d50f9eb3cab447ab4980c89f320e2 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a39c8c6  #30393 work around sphinx issue

comment:21 Changed 2 years ago by
Status:  needs_work → needs_review 

comment:22 followup: 24 Changed 2 years ago by
I found the correct fix. In ReStructuredText syntax, one underscore at the end creates a target arb documentation
which needs to be unique in the page while two underscores doesn't create such a target. See: https://stackoverflow.com/questions/27420317/restructuredtextrsthttplinksunderscorevsuse
See the `Arb documentation <http://arblib.org/acb_mat.html#c.acb_mat_eig_multiple>`_ See the `Arb documentation <http://arblib.org/acb_mat.html#c.acb_mat_approx_eig_qr>`_ See the `Arb documentation <http://arblib.org/acb_mat.html#c.acb_mat_eig_simple>`_ +See the `Arb documentation <http://arblib.org/acb_mat.html#c.acb_mat_eig_multiple>`__ +See the `Arb documentation <http://arblib.org/acb_mat.html#c.acb_mat_approx_eig_qr>`__ +See the `Arb documentation <http://arblib.org/acb_mat.html#c.acb_mat_eig_simple>`__
Do you want to revert it back to using two underscores?
comment:23 Changed 2 years ago by
Commit:  a39c8c65063d50f9eb3cab447ab4980c89f320e2 → b792cbd249c916a27eabf532ea85778f9e0b97a0 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b792cbd  #30393 fix external links in docstrings

comment:24 followup: 26 Changed 2 years ago by
Replying to slabbe:
I found the correct fix.
Oh, I didn't know about the difference, thank you!
Do you want to revert it back to using two underscores?
Done (but untested; I don't want to rebuild the doc right now).
comment:25 Changed 2 years ago by
Status:  needs_review → positive_review 

and I checked that documentation builds ok.
comment:26 Changed 2 years ago by
Replying to mmezzarobba:
Replying to slabbe:
I found the correct fix.
Oh, I didn't know about the difference, thank you!
I created #30444 to add it to the documentation.
comment:27 Changed 2 years ago by
Branch:  u/mmezzarobba/acb_eig → b792cbd249c916a27eabf532ea85778f9e0b97a0 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:28 Changed 2 years ago by
Commit:  b792cbd249c916a27eabf532ea85778f9e0b97a0 

While following the examples in the documentation in order to prepare something for the release tours, I noticed that the computed right eigenvectors are wrong. They do not satisfy the eigenvector equation. In fact, it seems to be a problem of transposition. The computed vectors are the rows of the eigenmatrix, but the eigenvectors would be the columns.
sage: from sage.matrix.benchmark import hilbert_matrix sage: mat = hilbert_matrix(3).change_ring(CBF) sage: [v[1][0].column() for v in mat.eigenvectors_right()] ⎡ ⎛ [0.827044926972 +/ 1.17e13] + [+/ 1.08e13]*I⎞ ⎛[0.459863904366 +/ 5.79e13] + [+/ 1.22e13]*I⎞ ⎢ ⎜[0.547448430721 +/ 4.33e13] + [+/ 1.08e13]*I⎟ ⎜[0.528290235067 +/ 5.58e13] + [+/ 1.22e13]*I⎟ ⎣ ⎝[0.127659329747 +/ 5.74e13] + [+/ 1.08e13]*I⎠, ⎝[0.713746885803 +/ 5.34e13] + [+/ 1.22e13]*I⎠, ⎛ [0.323298435244 +/ 6.18e13] + [+/ 1.19e13]*I⎞ ⎤ ⎜ [0.649006658852 +/ 4.06e13] + [+/ 1.19e13]*I⎟ ⎥ ⎝[0.688671531671 +/ 4.92e13] + [+/ 1.19e13]*I⎠ ⎦ sage: mat.change_ring(CDF).eigenmatrix_right()[1] ⎛ 0.8270449269720096 0.5474484307206746 0.12765932974653435⎞ ⎜ 0.45986390436554375 0.5282902350674367 0.713746885803413⎟ ⎝ 0.32329843524449886 0.6490066588517125 0.6886715316713734⎠
comment:29 Changed 2 years ago by
Uh, thanks a lot for catching the issue! I thought I has checked...
New commits:
#30393 wrap arb's experimental eigensolver