Opened 2 years ago

Closed 2 years ago

#24742 closed enhancement (fixed)

New MatrixArgs object to deal with constructing matrices

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.2
Component: linear algebra Keywords:
Cc: tscrim, vdelecroix, embray Merged in:
Authors: Jeroen Demeyer Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: bf9cefd (Commits) Commit: bf9cefd138bb9fbb19c3c43a778ab230bf962902
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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

  1. The global matrix() constructor
  1. MatrixSpace.__call__
  1. 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.
  1. 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.
  1. 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

Change History (73)

comment:1 Changed 2 years ago by jdemeyer

  • Cc tscrim vdelecroix added
  • Description modified (diff)

comment:2 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/new_matrixinput_object_to_deal_with_constructing_matrices

comment:4 Changed 2 years ago by git

  • Commit set to de8775b89e49cccf4893ef515b50eea3afd9d375

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

de8775bNew MatrixInput object to deal with constructing matrices

comment:5 Changed 2 years ago by git

  • Commit changed from de8775b89e49cccf4893ef515b50eea3afd9d375 to e02c31aebd10b85b016bbcf1536330c59685fa29

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

e02c31aNew MatrixInput object to deal with constructing matrices

comment:6 Changed 2 years ago by git

  • Commit changed from e02c31aebd10b85b016bbcf1536330c59685fa29 to 399b2bbee6c63b71c537087a8af1d59e80890947

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

399b2bbNew MatrixInput object to deal with constructing matrices

comment:7 Changed 2 years ago by git

  • Commit changed from 399b2bbee6c63b71c537087a8af1d59e80890947 to 7f18749fd1c45eab5b3c2ca2633233d76e2d7e6a

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

7f18749New MatrixInput object to deal with constructing matrices

comment:8 Changed 2 years ago by git

  • Commit changed from 7f18749fd1c45eab5b3c2ca2633233d76e2d7e6a to 83c09c35b606df47b451bbfe938bc79148220e38

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

83c09c3New MatrixInput object to deal with constructing matrices

comment:9 Changed 2 years ago by git

  • Commit changed from 83c09c35b606df47b451bbfe938bc79148220e38 to 6d91682a1c80f7f83ea86e3b6018ae2a5e4ab5b5

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

853d575New MatrixInput object to deal with constructing matrices
6d91682Remove import of matrix.special in matrix.constructor

comment:10 Changed 2 years ago by jdemeyer

  • Summary changed from New MatrixInput object to deal with constructing matrices to New MatrixArgs object to deal with constructing matrices

comment:11 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:12 Changed 2 years ago by git

  • Commit changed from 6d91682a1c80f7f83ea86e3b6018ae2a5e4ab5b5 to 1ed6a49b23be9a7fd4a25cbcaa98b178ba49756f

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

ddc96f5Stop using prepare_dict()
1ed6a49New MatrixArgs object to deal with constructing matrices

comment:13 Changed 2 years ago by git

  • Commit changed from 1ed6a49b23be9a7fd4a25cbcaa98b178ba49756f to 0ce108f00fd94b71a8e384c98c9e96db30eaae92

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

0ce108fNew MatrixArgs object to deal with constructing matrices

comment:14 Changed 2 years ago by git

  • Commit changed from 0ce108f00fd94b71a8e384c98c9e96db30eaae92 to ddff20fd1b009e38b9337031882c9c89d362fc62

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

ddff20fNew MatrixArgs object to deal with constructing matrices

comment:15 Changed 2 years ago by git

  • Commit changed from ddff20fd1b009e38b9337031882c9c89d362fc62 to ec0c619abda3a2fe16c6cd3e36c0b38210ae59d2

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

ec0c619New MatrixArgs object to deal with constructing matrices

comment:16 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:17 Changed 2 years ago by embray

  • Cc embray added

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

comment:18 Changed 2 years ago by embray

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.

comment:19 Changed 2 years ago by git

  • Commit changed from ec0c619abda3a2fe16c6cd3e36c0b38210ae59d2 to a0f37724d92c0810c4b37c00625e9e404d728de3

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

36b2456Stop using prepare_dict()
a0f3772New MatrixArgs object to deal with constructing matrices

comment:20 Changed 2 years ago by jdemeyer

  • Dependencies set to #24814

comment:21 Changed 2 years ago by git

  • Commit changed from a0f37724d92c0810c4b37c00625e9e404d728de3 to 769af020c25a7dc4f997289e8fdc759a0469c02a

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

769af02New MatrixArgs object to deal with constructing matrices

comment:22 Changed 2 years ago by git

  • Commit changed from 769af020c25a7dc4f997289e8fdc759a0469c02a to 6b8aed036ea1e844ae9b6716ec180dde29a873e4

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

f3497c3Stop using prepare_dict()
6b8aed0New MatrixArgs object to deal with constructing matrices

comment:23 Changed 2 years ago by git

  • Commit changed from 6b8aed036ea1e844ae9b6716ec180dde29a873e4 to 9a1d7b0fbdfd9ed6b20db7e62433d797b953ed33

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

12d5c52Implement length-checking iterator
9a1d7b0New MatrixArgs object to deal with constructing matrices

comment:24 Changed 2 years ago by jdemeyer

  • Dependencies changed from #24814 to #24814, #24828

comment:25 Changed 2 years ago by git

  • Commit changed from 9a1d7b0fbdfd9ed6b20db7e62433d797b953ed33 to 3b77656e2a0ced9b6a25fd837bd1ace8ee47cf0e

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

33fe7fcNew MatrixArgs object to deal with constructing matrices
3b77656zero_matrix() should pass the correct zero

comment:26 Changed 2 years ago by jdemeyer

  • Dependencies changed from #24814, #24828 to #24814, #24828, #24829

comment:27 Changed 2 years ago by git

  • Commit changed from 3b77656e2a0ced9b6a25fd837bd1ace8ee47cf0e to 51105fb09a9daaef4aa82f32b97d3347d655f83c

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

0201afaImplement length-checking iterator
3682bf2Stop using prepare_dict()
7ca2a92zero_matrix() should pass the correct zero
51105fbNew MatrixArgs object to deal with constructing matrices

comment:28 Changed 2 years ago by git

  • Commit changed from 51105fb09a9daaef4aa82f32b97d3347d655f83c to 333173b4fe37a049d835bb7353b9d124045f99cf

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

333173bNew MatrixArgs object to deal with constructing matrices

comment:29 Changed 2 years ago by git

  • Commit changed from 333173b4fe37a049d835bb7353b9d124045f99cf to a7146ce1f3ca556bb9ded20e0c7c0a04ace0f40a

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

9e54f93Avoid importing matrices in polynomial rings
a7146ceNew MatrixArgs object to deal with constructing matrices

comment:30 Changed 2 years ago by git

  • Commit changed from a7146ce1f3ca556bb9ded20e0c7c0a04ace0f40a to ddf4b98a76b8bc867ae957bd786fba7a50654c0d

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

ddf4b98New MatrixArgs object to deal with constructing matrices

comment:31 Changed 2 years ago by git

  • Commit changed from ddf4b98a76b8bc867ae957bd786fba7a50654c0d to eff01197b87fba2ff81f85de4351ee67df8cbe79

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

eff0119New MatrixArgs object to deal with constructing matrices

comment:32 Changed 2 years ago by jdemeyer

  • Dependencies changed from #24814, #24828, #24829 to #24814, #24828, #24829, #24863

comment:33 Changed 2 years ago by jdemeyer

  • Dependencies changed from #24814, #24828, #24829, #24863 to #24814, #24828, #24829, #24863, #24865

comment:34 Changed 2 years ago by git

  • Commit changed from eff01197b87fba2ff81f85de4351ee67df8cbe79 to 390e49e328296309a4a4736f97d49c6a8cbc4e43

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

883dd1fFix signature of Matrix_gfpn_dense.__init__
390e49eNew MatrixArgs object to deal with constructing matrices

comment:35 Changed 2 years ago by git

  • Commit changed from 390e49e328296309a4a4736f97d49c6a8cbc4e43 to ce371891a6d80333ed4e502b472d8597230ca031

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

f79bb50Finite field elements should not have a _matrix_ method
ce37189New MatrixArgs object to deal with constructing matrices

comment:36 Changed 2 years ago by git

  • Commit changed from ce371891a6d80333ed4e502b472d8597230ca031 to b024a7ebfa89bac922c129f29139b3265212c75a

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

0d7971dFix a few matrix doctests
b024a7eNew MatrixArgs object to deal with constructing matrices

comment:37 Changed 2 years ago by git

  • Commit changed from b024a7ebfa89bac922c129f29139b3265212c75a to 6bdfb8112bfbf99b6368e7268eb548761ffea2fe

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

d17395aMinor fixes involving matrices
6bdfb81New MatrixArgs object to deal with constructing matrices

comment:38 Changed 2 years ago by jdemeyer

  • Dependencies changed from #24814, #24828, #24829, #24863, #24865 to #24814, #24828, #24829, #24863, #24865, #24874

comment:39 Changed 2 years ago by git

  • Commit changed from 6bdfb8112bfbf99b6368e7268eb548761ffea2fe to 4944cd47e704c58554fde0a198084651266cdc8c

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

9587999BooleanMonomialMonoid is commutative
1777b44Minor fixes involving matrices
4944cd4New MatrixArgs object to deal with constructing matrices

comment:40 Changed 2 years ago by git

  • Commit changed from 4944cd47e704c58554fde0a198084651266cdc8c to 4ef5dc20076d91fdd84fd6ee6a37e50d258a4ee9

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

9e7fbb6Minor fixes involving matrices
4ef5dc2New MatrixArgs object to deal with constructing matrices

comment:41 Changed 2 years ago by jdemeyer

  • Description modified (diff)

New commits:

9e7fbb6Minor fixes involving matrices
4ef5dc2New MatrixArgs object to deal with constructing matrices

comment:42 Changed 2 years ago by git

  • Commit changed from 4ef5dc20076d91fdd84fd6ee6a37e50d258a4ee9 to d5c9ee0593755e3effad20b02b8f920ea2c799f3

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

90141c8Minor fixes involving matrices
d5c9ee0New MatrixArgs object to deal with constructing matrices

comment:43 Changed 2 years ago by git

  • Commit changed from d5c9ee0593755e3effad20b02b8f920ea2c799f3 to 3f722cfe6b66631a20e9872d3a3db2515849a670

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

8d2db12Minor fixes involving matrices
3f722cfNew MatrixArgs object to deal with constructing matrices

comment:44 Changed 2 years ago by jdemeyer

  • Dependencies changed from #24814, #24828, #24829, #24863, #24865, #24874 to #24814, #24828, #24829, #24863, #24865, #24874, #24881

comment:45 Changed 2 years ago by git

  • Commit changed from 3f722cfe6b66631a20e9872d3a3db2515849a670 to f190e72c0c8a487469a41321dcff31a01f733c82

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

4f6c776Matrix-related fixes in differential geometry
f190e72New MatrixArgs object to deal with constructing matrices

comment:46 Changed 2 years ago by jdemeyer

  • Dependencies changed from #24814, #24828, #24829, #24863, #24865, #24874, #24881 to #24814, #24828, #24829, #24863, #24865, #24874, #24881, #24884

comment:47 Changed 2 years ago by git

  • Commit changed from f190e72c0c8a487469a41321dcff31a01f733c82 to b9ee50f3dd0b20ae8bd26606ad63363b05033350

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

b9ee50fNew MatrixArgs object to deal with constructing matrices

comment:48 Changed 2 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from new to needs_review

comment:49 Changed 2 years ago by git

  • Commit changed from b9ee50f3dd0b20ae8bd26606ad63363b05033350 to 0cc0d5d2392ae97ebb0a28608a0c8494a3f0f64a

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

0cc0d5dNew MatrixArgs object to deal with constructing matrices

comment:50 Changed 2 years ago by jdemeyer

  • Dependencies changed from #24814, #24828, #24829, #24863, #24865, #24874, #24881, #24884 to #24828, #24863, #24865, #24874, #24881, #24884

comment:51 Changed 2 years ago by git

  • Commit changed from 0cc0d5d2392ae97ebb0a28608a0c8494a3f0f64a to 850a2fae83a3b15edae004ac85695130c76a31b3

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

eb0ce2aImprove treatment of matrix output in sage.tensor.modules.comp.Components._get_list.
850a2faNew MatrixArgs object to deal with constructing matrices

comment:52 Changed 2 years ago by git

  • Commit changed from 850a2fae83a3b15edae004ac85695130c76a31b3 to f5a7ddbabed7da0e983e80250073d7dfcf56b601

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

30b2cb4Implement length-checking iterator
22a1630Avoid importing matrices in polynomial rings
f5a7ddbNew MatrixArgs object to deal with constructing matrices

comment:53 Changed 2 years ago by git

  • Commit changed from f5a7ddbabed7da0e983e80250073d7dfcf56b601 to 1ba4b76970b0f3d63404a7c4f28d026f08e7bcec

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

0b4bdd8Avoid importing matrices in polynomial rings
d527770Implement length-checking iterator
1ba4b76New MatrixArgs object to deal with constructing matrices

comment:54 Changed 2 years ago by jdemeyer

  • Dependencies changed from #24828, #24863, #24865, #24874, #24881, #24884 to #24828, #24945

comment:55 follow-up: Changed 2 years ago by mmezzarobba

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.

comment:56 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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.

comment:57 Changed 2 years ago by jdemeyer

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

comment:58 Changed 2 years ago by jdemeyer

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

comment:59 Changed 2 years ago by git

  • Commit changed from 1ba4b76970b0f3d63404a7c4f28d026f08e7bcec to fac3f5e2aec876cfae4ec8c0bc18879a19cd001d

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

447b4e5Properly fix signature of Matrix_gfpn_dense.__init__
f14395cUse a lazy_attribute for _Karatsuba_threshold
4397945Implement length-checking iterator
fac3f5eNew MatrixArgs object to deal with constructing matrices

comment:60 Changed 2 years ago by jdemeyer

  • Dependencies changed from #24828, #24945 to #24828, #24947, #24945
  • Status changed from needs_work to needs_review

comment:61 in reply to: ↑ 55 Changed 2 years ago by jdemeyer

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.

comment:62 Changed 2 years ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

comment:63 Changed 2 years ago by git

  • Commit changed from fac3f5e2aec876cfae4ec8c0bc18879a19cd001d to bf9cefd138bb9fbb19c3c43a778ab230bf962902
  • Status changed from positive_review to needs_review

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

bf9cefdNew MatrixArgs object to deal with constructing matrices

comment:64 Changed 2 years ago by jdemeyer

  • Dependencies #24828, #24947, #24945 deleted
  • Description modified (diff)
  • Status changed from needs_review to positive_review

Trivially rebased.

comment:65 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:66 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:67 follow-up: Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

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

comment:68 in reply to: ↑ 67 Changed 2 years ago by jdemeyer

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?

comment:69 Changed 2 years ago by jdemeyer

  • Status changed from needs_work to positive_review

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

comment:70 Changed 2 years ago by nbruin

  • Status changed from positive_review to needs_work

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

comment:71 Changed 2 years ago by jdemeyer

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

comment:72 Changed 2 years ago by jdemeyer

  • Status changed from needs_work to positive_review

comment:73 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/new_matrixinput_object_to_deal_with_constructing_matrices to bf9cefd138bb9fbb19c3c43a778ab230bf962902
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.