Opened 2 years ago

Closed 2 years ago

Last modified 10 months ago

#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:

Status badges

Description (last modified by mmezzarobba)

The wrappers are marked as experimental because the corresponding arb functions are.

Change History (32)

comment:1 Changed 2 years ago by mmezzarobba

Branch: u/mmezzarobba/acb_eig
Commit: 1d42e2b2b1142a1ec7ea2cc8adb282521c94120e
Description: modified (diff)
Status: newneeds_review

New commits:

1d42e2b#30393 wrap arb's experimental eigensolver

comment:2 Changed 2 years ago by slabbe

Note that the signature of that method is changing in #29243. Maybe better to follow that?

comment:3 Changed 2 years ago by git

Commit: 1d42e2b2b1142a1ec7ea2cc8adb282521c94120ee89ac6348761059cf038dbfdcc4436b987b633de

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

e89ac63#30393 match the new interface introduced in #29243

comment:4 in reply to:  2 Changed 2 years ago by mmezzarobba

Replying to slabbe:

Note that the signature of that method is changing in #29243. Maybe better to follow that?

Thank you. I'm not setting that ticket as a dependency though, since these methods are experimental anyway.

Last edited 2 years ago by mmezzarobba (previous) (diff)

comment:5 Changed 2 years ago by git

Commit: e89ac6348761059cf038dbfdcc4436b987b633de56f22b122b2df7ce7757b01ee4ef13ac1922066a

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 Changed 2 years ago by slabbe

Few comments:

  • "unable to certify the eigenvalues of this interval matrix", I would avoid the usage of the word interval which reminds of the CIF since the matrix belongs rather to the CBF.
  • the other input should be documented as related to the generalized eigenvalue problem in the input block and/or in the raise NotImplementedError.

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 git

Commit: 56f22b122b2df7ce7757b01ee4ef13ac1922066a7f1b45ef37d635b6017566f90b28c94a9517d231

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 in reply to:  6 ; Changed 2 years ago by mmezzarobba

Replying to slabbe:

Few comments:

  • "unable to certify the eigenvalues of this interval matrix", I would avoid the usage of the word interval which reminds of the CIF since the matrix belongs rather to the CBF.
  • the other input should be documented as related to the generalized eigenvalue problem in the input block and/or in the raise NotImplementedError.

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 git

Commit: 7f1b45ef37d635b6017566f90b28c94a9517d2315f6fb40362912ff5870e3e5f76abbd902243ee17

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 in reply to:  8 ; Changed 2 years ago by slabbe

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 Changed 2 years ago by 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".

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.
Last edited 2 years ago by slabbe (previous) (diff)

comment:12 Changed 2 years ago by slabbe

Status: needs_reviewneeds_work

comment:13 Changed 2 years ago by git

Commit: 5f6fb40362912ff5870e3e5f76abbd902243ee17edd1ae05e84205894df598c2b77ad45b9a2fdd58

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

edd1ae0#30393 work around sphinx issue

comment:14 in reply to:  10 Changed 2 years ago by mmezzarobba

Replying to slabbe:

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?

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 in reply to:  11 Changed 2 years ago by mmezzarobba

Status: needs_workneeds_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 slabbe

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 slabbe

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
Last edited 2 years ago by slabbe (previous) (diff)

comment:18 Changed 2 years ago by slabbe

Status: needs_reviewneeds_work

comment:19 Changed 2 years ago by slabbe

Reviewers: Sébastien Labbé

comment:20 Changed 2 years ago by git

Commit: edd1ae05e84205894df598c2b77ad45b9a2fdd58a39c8c65063d50f9eb3cab447ab4980c89f320e2

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 mmezzarobba

Status: needs_workneeds_review

Oops. I went too fast indeed, thanks!


New commits:

a39c8c6#30393 work around sphinx issue

comment:22 Changed 2 years ago by slabbe

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 git

Commit: a39c8c65063d50f9eb3cab447ab4980c89f320e2b792cbd249c916a27eabf532ea85778f9e0b97a0

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 in reply to:  22 ; Changed 2 years ago by mmezzarobba

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 slabbe

Status: needs_reviewpositive_review

and I checked that documentation builds ok.

comment:26 in reply to:  24 Changed 2 years ago by slabbe

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 vbraun

Branch: u/mmezzarobba/acb_eigb792cbd249c916a27eabf532ea85778f9e0b97a0
Resolution: fixed
Status: positive_reviewclosed

comment:28 Changed 2 years ago by gh-mwageringel

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 mmezzarobba

Uh, thanks a lot for catching the issue! I thought I has checked...

comment:30 Changed 2 years ago by mmezzarobba

Fixed (I hope) at #30568.

comment:31 Changed 2 years ago by slabbe

Oups, I didn't notice that issue during the review. Thanks.

comment:32 Changed 10 months ago by slabbe

Another follow up: #33652

Note: See TracTickets for help on using tickets.