#30393 closed enhancement (fixed)
eigenvalues and eigenvectors using arb
Reported by: | mmezzarobba | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.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 follow-up: 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 follow-up: 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 follow-up: 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 follow-up: 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 follow-up: 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 follow-up: 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/restructured-text-rst-http-links-underscore-vs-use
-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 follow-up: 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.17e-13] + [+/- 1.08e-13]*I⎞ ⎛[0.459863904366 +/- 5.79e-13] + [+/- 1.22e-13]*I⎞ ⎢ ⎜[-0.547448430721 +/- 4.33e-13] + [+/- 1.08e-13]*I⎟ ⎜[0.528290235067 +/- 5.58e-13] + [+/- 1.22e-13]*I⎟ ⎣ ⎝[-0.127659329747 +/- 5.74e-13] + [+/- 1.08e-13]*I⎠, ⎝[0.713746885803 +/- 5.34e-13] + [+/- 1.22e-13]*I⎠, ⎛ [0.323298435244 +/- 6.18e-13] + [+/- 1.19e-13]*I⎞ ⎤ ⎜ [0.649006658852 +/- 4.06e-13] + [+/- 1.19e-13]*I⎟ ⎥ ⎝[-0.688671531671 +/- 4.92e-13] + [+/- 1.19e-13]*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