New MatrixArgs object to deal with constructing matrices

[Jeroen Demeyer]

Constructing matrices is a mess currently. There is a lot of code duplication between

1. The global matrix() constructor

2. MatrixSpace.__call__

3. The various Matrix.__init__ methods

Since a lot of parameters are involved (matrix space, matrix size, entries, ring, sparseness, implementation), I decided to create a new class MatrixArgs just for dealing with inputting matrices. This also makes matrices more consistent: because the same code is used everywhere, constructing matrices should now behave the same independently of the implementation (base ring, sparseness).

These are the changes in this branch:

1. A completely new module sage.matrix.args. This module concentrates in one place everything which was scattered in various places before. I tried to reproduce the existing functionality, except where it didn't make sense. I took care to be efficient where efficiency matters, for example by avoiding copies.

2. Enormous simplifications to A, B and C above, typically replacing them with just a few lines of code. In a few cases where the original code was particularly ugly, I mostly left the ugly code in place to be fixed later.

3. Various minor changes here and there. Note that most of these were separated in dependent tickets, but a few are left for this ticket.

This fixes: #8277, #10613, #12778, #20211, #19134, #25076

Follow-up tickets: #23719, #25061

[git]

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

​de8775b

New MatrixInput object to deal with constructing matrices

[git]

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

​e02c31a

New MatrixInput object to deal with constructing matrices

[git]

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

​399b2bb

New MatrixInput object to deal with constructing matrices

[git]

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

​7f18749

New MatrixInput object to deal with constructing matrices

[git]

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

​83c09c3

New MatrixInput object to deal with constructing matrices

[git]

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

​853d575	New MatrixInput object to deal with constructing matrices
​6d91682	Remove import of matrix.special in matrix.constructor

[git]

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

​ddc96f5	Stop using prepare_dict()
​1ed6a49	New MatrixArgs object to deal with constructing matrices

[git]

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

​0ce108f

New MatrixArgs object to deal with constructing matrices

[git]

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

​ddff20f

New MatrixArgs object to deal with constructing matrices

[git]

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

​ec0c619

New MatrixArgs object to deal with constructing matrices

[Erik Bray]

Haven't ready it 100% yet but I get the idea. Definitely looks like an improvement.

[Erik Bray]

One very minor comment:

+    def __repr__(self):
+        """
+        Print representation for debugging
+        """
+        t = "???"

I know this is mainly just for debugging, but I think instead of "???" as the default something more explicit like "<unfinalized>" or "<invalid>" would be clearer.

[git]

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

​36b2456	Stop using prepare_dict()
​a0f3772	New MatrixArgs object to deal with constructing matrices

[git]

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

​769af02

New MatrixArgs object to deal with constructing matrices

[git]

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

​f3497c3	Stop using prepare_dict()
​6b8aed0	New MatrixArgs object to deal with constructing matrices

[git]

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

​12d5c52	Implement length-checking iterator
​9a1d7b0	New MatrixArgs object to deal with constructing matrices

[git]

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

​33fe7fc	New MatrixArgs object to deal with constructing matrices
​3b77656	zero_matrix() should pass the correct zero

[git]

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

​0201afa	Implement length-checking iterator
​3682bf2	Stop using prepare_dict()
​7ca2a92	zero_matrix() should pass the correct zero
​51105fb	New MatrixArgs object to deal with constructing matrices

[git]

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

​333173b

New MatrixArgs object to deal with constructing matrices

[git]

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

​9e54f93	Avoid importing matrices in polynomial rings
​a7146ce	New MatrixArgs object to deal with constructing matrices

[git]

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

​ddf4b98

New MatrixArgs object to deal with constructing matrices

[git]

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

​eff0119

New MatrixArgs object to deal with constructing matrices

[git]

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

​883dd1f	Fix signature of Matrix_gfpn_dense.__init__
​390e49e	New MatrixArgs object to deal with constructing matrices

[git]

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

​f79bb50	Finite field elements should not have a _matrix_ method
​ce37189	New MatrixArgs object to deal with constructing matrices

[git]

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

​0d7971d	Fix a few matrix doctests
​b024a7e	New MatrixArgs object to deal with constructing matrices

[git]

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

​d17395a	Minor fixes involving matrices
​6bdfb81	New MatrixArgs object to deal with constructing matrices

[git]

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

​9587999	BooleanMonomialMonoid is commutative
​1777b44	Minor fixes involving matrices
​4944cd4	New MatrixArgs object to deal with constructing matrices

[git]

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

​9e7fbb6	Minor fixes involving matrices
​4ef5dc2	New MatrixArgs object to deal with constructing matrices

[Jeroen Demeyer]

New commits:

​9e7fbb6	Minor fixes involving matrices
​4ef5dc2	New MatrixArgs object to deal with constructing matrices

[git]

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

​90141c8	Minor fixes involving matrices
​d5c9ee0	New MatrixArgs object to deal with constructing matrices

[git]

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

​8d2db12	Minor fixes involving matrices
​3f722cf	New MatrixArgs object to deal with constructing matrices

[git]

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

​4f6c776	Matrix-related fixes in differential geometry
​f190e72	New MatrixArgs object to deal with constructing matrices

[git]

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

​b9ee50f

New MatrixArgs object to deal with constructing matrices

[git]

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

​0cc0d5d

New MatrixArgs object to deal with constructing matrices

[git]

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

​eb0ce2a	Improve treatment of matrix output in sage.tensor.modules.comp.Components._get_list.
​850a2fa	New MatrixArgs object to deal with constructing matrices

[git]

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

​30b2cb4	Implement length-checking iterator
​22a1630	Avoid importing matrices in polynomial rings
​f5a7ddb	New MatrixArgs object to deal with constructing matrices

[git]

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

​0b4bdd8	Avoid importing matrices in polynomial rings
​d527770	Implement length-checking iterator
​1ba4b76	New MatrixArgs object to deal with constructing matrices

[Marc Mezzarobba]

Very nice work! Though I honestly don't understand the new code in detail, it looks reasonable and I trust the tests more than my own judgment to check that it does the right thing. Even if there are small issues, I think it can only be an improvement compared to what we had before.

I have one significant concern with this ticket though: several simple cases of matrix(...) apparently became a lot slower. Compare in particular:

Sage 8.1:

sage: %timeit matrix(ZZ,3,3)[0,0] = 1 # used to be a fast way to construct a new mutable matrix
The slowest run took 62.41 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 7 µs per loop
sage: c = vector(ZZ, [1,2,3])
sage: %timeit matrix([c])
The slowest run took 4.77 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 33.7 µs per loop

This ticket:

sage: %timeit matrix(ZZ,3,3)[0,0] = 1
10000 loops, best of 3: 110 µs per loop
sage: c = vector(ZZ, [1,2,3])
sage: %timeit matrix([c])
10000 loops, best of 3: 101 µs per loop

Some nitpicking while I'm at it: it is perhaps not ideal that

sage: matrix(1,1,[[1,2]])
ValueError: inconsistent number of columns: should be 1 but got 2

and

sage: matrix(ZZ,1,1,[[1,2]])
ValueError: sequence too long (expected length 1, got more)

yield different errors.

[Jeroen Demeyer]

There is something very strange going on with the timings:

With this ticket, constructing a matrix and storing it is an order of magnitude faster than constructing a matrix and not storing it:

sage: timeit('matrix(3, 3)', repeat=5, number=10000)
10000 loops, best of 5: 117 µs per loop
sage: timeit('a=matrix(3, 3)', repeat=5, number=10000)
10000 loops, best of 5: 10.7 µs per loop

It doesn't matter if the storing happens inside the timeit() or not:

sage: M = matrix(3,3)
sage: timeit('matrix(3, 3)', repeat=5, number=10000)
10000 loops, best of 5: 10.5 µs per loop
sage: del M
sage: timeit('matrix(3, 3)', repeat=5, number=10000)
10000 loops, best of 5: 115 µs per loop

There must be some caching issue here...

The current behaviour (without #24742) does not have this.

[Jeroen Demeyer]

When a previous matrix is "cached", the timings with this ticket are actually better than before.

Before (8.2.beta8):

sage: M = matrix(ZZ,3,3)
sage: timeit('matrix(ZZ,3,3)', repeat=5, number=10000)
10000 loops, best of 5: 13.2 µs per loop

sage: c = vector(ZZ, [1,2,3]); matrix([c])
[1 2 3]
sage: timeit('matrix([c])', repeat=5, number=10000)
10000 loops, best of 5: 46.3 µs per loop

After (#24742):

sage: M = matrix(ZZ,3,3)
sage: timeit('matrix(ZZ,3,3)', repeat=5, number=10000)
10000 loops, best of 5: 10.7 µs per loop

sage: c = vector(ZZ, [1,2,3]); matrix([c])
[1 2 3]
sage: timeit('matrix([c])', repeat=5, number=10000)
10000 loops, best of 5: 14.5 µs per loop

[Jeroen Demeyer]

The slowdown within timeit() is tracked at #24954.

[git]

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

​447b4e5	Properly fix signature of Matrix_gfpn_dense.__init__
​f14395c	Use a lazy_attribute for _Karatsuba_threshold
​4397945	Implement length-checking iterator
​fac3f5e	New MatrixArgs object to deal with constructing matrices

[Jeroen Demeyer]

Replying to mmezzarobba:

Some nitpicking while I'm at it: it is perhaps not ideal that

sage: matrix(1,1,[[1,2]])
ValueError: inconsistent number of columns: should be 1 but got 2

and

sage: matrix(ZZ,1,1,[[1,2]])
ValueError: sequence too long (expected length 1, got more)

yield different errors.

Not ideal, but it's not clear what the best solution is. The problem is that I'm using different code paths for the case where everything (base ring + matrix dimensions) is known and for when it's not known. In order to determine the base ring, I have to iterate over the entries anyway. It's there that the error "inconsistent number of columns" is raised.

[git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

​bf9cefd

New MatrixArgs object to deal with constructing matrices

[Jeroen Demeyer]

Trivially rebased.

[Volker Braun]

Failures in src/sage/schemes/riemann_surfaces/riemann_surface.py, possibly due to 24370

[Jeroen Demeyer]

Replying to vbraun:

Failures in src/sage/schemes/riemann_surfaces/riemann_surface.py, possibly due to 24370

If #24370 and #24742 conflict, can we instead set #24370 to needs_work?

[Jeroen Demeyer]

I think that #24370 is the ticket that deserved to be set to needs_work, but I'll hear from you if you disagree.

[Nils Bruin]

Apparently a conflict with #24370. These tickets just need to be calibrated. The fix is clear either way.

[Jeroen Demeyer]

With the updated version of #24370, everything works fine! So both tickets can get positive review.