More substantial implementation of matrices over complex ball fields

[mezzarobba]

Depends on #24742

CC: @fredrik-johansson @videlec @cheuberg

Component: linear algebra

Author: Marc Mezzarobba

Branch/Commit: 13b8b8f

Reviewer: Vincent Delecroix

Issue created by migration from https://trac.sagemath.org/ticket/24626

[videlec]

comment:3

Given that comparison of balls is modified in #24627 to use directly arb, I don't understand the following

+        if op == Py_EQ:
+            return lt is rt or acb_mat_eq(lt.value, rt.value)

[videlec]

comment:4

What should be the output of the two things below?

sage: a = matrix(CBF, [1/3])
sage: b = matrix(CBF, [1/3])
sage: (a == a) == (a[0,0] == a[0,0])
sage: (a == b) == (a[0,0] == b[0,0])

(such kind of tests would be good as doctests)

[videlec]

comment:5

Instead of

elif op == Py_GT or op == Py_GE or op == Py_LT or op == Py_LE:

you can use else (and not import Py_GT, Py_GE, Py_LT, Py_LE)

[videlec]

comment:6

You would better test +, -, ...rather than _add_, _sub_, ...

[videlec]

comment:7

(BTW, why __invert__(self) (two underscores, standard Python) and _neg_(self) (where does it come from!?))

[mezzarobba]

comment:8

Thanks for you comments!

Replying to @videlec:

Given that comparison of balls is modified in #24627 to use directly arb, I don't understand the following

+        if op == Py_EQ:
+            return lt is rt or acb_mat_eq(lt.value, rt.value)

Good catch. The explanation is simply that this ticket is older than #24627.

Replying to @videlec:

Instead of

elif op == Py_GT or op == Py_GE or op == Py_LT or op == Py_LE:

you can use else (and not import Py_GT, Py_GE, Py_LT, Py_LE)

Yes, but I prefer the current version. (Is there any guarantee that Python won't add more comparison operators, btw?)

Replying to @videlec:

You would better test +, -, ...rather than _add_, _sub_, ...

Why? What I want in this case in really to test the implementation for this specific parent, not the dispatch done by the coercion framework.

Replying to @videlec:

(BTW, why __invert__(self) (two underscores, standard Python) and _neg_(self) (where does it come from!?))

Element implements __neg__() and not __invert__(), probably because abusing __invert__() to mean 1/x like most Sage Elements do may not be such a good idea. But I don't think there is any reason to implement _neg_() rather than __neg__() in the present case, and the latter should be slightly faster.

[videlec]

Dependencies: #24627

[sagetrac-git]

Changed commit from a33f27d to bf2d6ac

[sagetrac-git]

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

16962e5	real_arb: use arb 2.6+ comparison functions
ac4ba37	complex_arb: use arb 2.6+ comparison functions
b55e3c2	stop considering inexact balls equal to themselves
bf2d6ac	Merge branch 'arb_cmp' into acb_mat

[sagetrac-git]

Changed commit from bf2d6ac to 093c7eb

[sagetrac-git]

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

093c7eb

#24626 use cmp semantics of #24627

[videlec]

comment:13

docstring in charpoly? The rest looks good.

[mezzarobba]

Changed dependencies from #24627 to #24742

[sagetrac-git]

Changed commit from 093c7eb to 9533111

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4397945	Implement length-checking iterator
fac3f5e	New MatrixArgs object to deal with constructing matrices
d956343	expand Matrix_complex_ball_dense
ede80a0	#24626 use cmp semantics of #24627
9533111	#24626 missing description in docstring

[mezzarobba]

comment:16

Replying to @videlec:

docstring in charpoly?

Fixed, thanks!

I also rebased the branch on top of fac3f5e (#24742) to avoid a merge conflict.

[videlec]

Changed commit from 9533111 to f306995

[videlec]

Changed branch from u/mmezzarobba/acb_mat to public/acb_mat

[videlec]

comment:17

merge conflicts with #24742 that has changed from fac3f5e to ​bf9cefd. I did the rebase.

---

New commits:

bf9cefd	New MatrixArgs object to deal with constructing matrices
e6542d6	expand Matrix_complex_ball_dense
b064606	#24626 use cmp semantics of #24627
f306995	#24626 missing description in docstring

[videlec]

comment:18

Replying to @mezzarobba:

Replying to @videlec:
Replying to @videlec:

Instead of

elif op == Py_GT or op == Py_GE or op == Py_LT or op == Py_LE:

you can use else (and not import Py_GT, Py_GE, Py_LT, Py_LE)

Yes, but I prefer the current version. (Is there any guarantee that Python won't add more comparison operators, btw?)

I don't understand why you would prefer the current version. And if Python does add operators your version will return None. You only want to support EQ and NE here, don't you?

[sagetrac-git]

Changed commit from f306995 to 01f6f87

[sagetrac-git]

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

01f6f87

#24626 `_neg_` -> __neg__

[videlec]

comment:20

waiting for more info on [comment:18]. The rest is ok.

[videlec]

Reviewer: Vincent Delecroix

[mezzarobba]

comment:22

Thanks for the rebase.

Replying to @videlec:

I don't understand why you would prefer the current version. And if Python does add operators your version will return None. You only want to support EQ and NE here, don't you?

Yes, probably. Go ahead and change it if you want--I find the version I wrote slightly clearer and I don't see the point of this discussion, but I don't really care.

[sagetrac-git]

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

13b8b8f

#24626 simplify `_richcmp_` and modify error msg

[sagetrac-git]

Changed commit from 01f6f87 to 13b8b8f

[videlec]

comment:24

if you are ok with the current state please set to positive review. I would like to move forward with #24927.

[mezzarobba]

comment:25

The failure reported by the patchbot is genuine, but not due to this ticket.

[vbraun]

Changed branch from public/acb_mat to 13b8b8f