Opened 3 years ago
Closed 3 years ago
#24742 closed enhancement (fixed)
New MatrixArgs object to deal with constructing matrices
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.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, GitHub, GitLab)  Commit:  bf9cefd138bb9fbb19c3c43a778ab230bf962902 
Dependencies:  Stopgaps: 
Description (last modified by )
Constructing matrices is a mess currently. There is a lot of code duplication between
 The global
matrix()
constructor
MatrixSpace.__call__
 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:
 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.
 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.
 Various minor changes here and there. Note that most of these were separated in dependent tickets, but a few are left for this ticket.
Change History (73)
comment:1 Changed 3 years ago by
 Cc tscrim vdelecroix added
 Description modified (diff)
comment:2 Changed 3 years ago by
 Description modified (diff)
comment:3 Changed 3 years ago by
 Branch set to u/jdemeyer/new_matrixinput_object_to_deal_with_constructing_matrices
comment:4 Changed 3 years ago by
 Commit set to de8775b89e49cccf4893ef515b50eea3afd9d375
comment:5 Changed 3 years ago by
 Commit changed from de8775b89e49cccf4893ef515b50eea3afd9d375 to e02c31aebd10b85b016bbcf1536330c59685fa29
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

comment:6 Changed 3 years ago by
 Commit changed from e02c31aebd10b85b016bbcf1536330c59685fa29 to 399b2bbee6c63b71c537087a8af1d59e80890947
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

comment:7 Changed 3 years ago by
 Commit changed from 399b2bbee6c63b71c537087a8af1d59e80890947 to 7f18749fd1c45eab5b3c2ca2633233d76e2d7e6a
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

comment:8 Changed 3 years ago by
 Commit changed from 7f18749fd1c45eab5b3c2ca2633233d76e2d7e6a to 83c09c35b606df47b451bbfe938bc79148220e38
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

comment:9 Changed 3 years ago by
 Commit changed from 83c09c35b606df47b451bbfe938bc79148220e38 to 6d91682a1c80f7f83ea86e3b6018ae2a5e4ab5b5
comment:10 Changed 3 years ago by
 Summary changed from New MatrixInput object to deal with constructing matrices to New MatrixArgs object to deal with constructing matrices
comment:11 Changed 3 years ago by
 Description modified (diff)
comment:12 Changed 3 years ago by
 Commit changed from 6d91682a1c80f7f83ea86e3b6018ae2a5e4ab5b5 to 1ed6a49b23be9a7fd4a25cbcaa98b178ba49756f
comment:13 Changed 3 years ago by
 Commit changed from 1ed6a49b23be9a7fd4a25cbcaa98b178ba49756f to 0ce108f00fd94b71a8e384c98c9e96db30eaae92
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

comment:14 Changed 3 years ago by
 Commit changed from 0ce108f00fd94b71a8e384c98c9e96db30eaae92 to ddff20fd1b009e38b9337031882c9c89d362fc62
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

comment:15 Changed 3 years ago by
 Commit changed from ddff20fd1b009e38b9337031882c9c89d362fc62 to ec0c619abda3a2fe16c6cd3e36c0b38210ae59d2
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

comment:16 Changed 3 years ago by
 Description modified (diff)
comment:17 Changed 3 years ago by
 Cc embray added
Haven't ready it 100% yet but I get the idea. Definitely looks like an improvement.
comment:18 Changed 3 years ago by
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 3 years ago by
 Commit changed from ec0c619abda3a2fe16c6cd3e36c0b38210ae59d2 to a0f37724d92c0810c4b37c00625e9e404d728de3
comment:20 Changed 3 years ago by
 Dependencies set to #24814
comment:21 Changed 3 years ago by
 Commit changed from a0f37724d92c0810c4b37c00625e9e404d728de3 to 769af020c25a7dc4f997289e8fdc759a0469c02a
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

comment:22 Changed 3 years ago by
 Commit changed from 769af020c25a7dc4f997289e8fdc759a0469c02a to 6b8aed036ea1e844ae9b6716ec180dde29a873e4
comment:23 Changed 3 years ago by
 Commit changed from 6b8aed036ea1e844ae9b6716ec180dde29a873e4 to 9a1d7b0fbdfd9ed6b20db7e62433d797b953ed33
comment:24 Changed 3 years ago by
 Dependencies changed from #24814 to #24814, #24828
comment:25 Changed 3 years ago by
 Commit changed from 9a1d7b0fbdfd9ed6b20db7e62433d797b953ed33 to 3b77656e2a0ced9b6a25fd837bd1ace8ee47cf0e
comment:26 Changed 3 years ago by
 Dependencies changed from #24814, #24828 to #24814, #24828, #24829
comment:27 Changed 3 years ago by
 Commit changed from 3b77656e2a0ced9b6a25fd837bd1ace8ee47cf0e to 51105fb09a9daaef4aa82f32b97d3347d655f83c
comment:28 Changed 3 years ago by
 Commit changed from 51105fb09a9daaef4aa82f32b97d3347d655f83c to 333173b4fe37a049d835bb7353b9d124045f99cf
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

comment:29 Changed 3 years ago by
 Commit changed from 333173b4fe37a049d835bb7353b9d124045f99cf to a7146ce1f3ca556bb9ded20e0c7c0a04ace0f40a
comment:30 Changed 3 years ago by
 Commit changed from a7146ce1f3ca556bb9ded20e0c7c0a04ace0f40a to ddf4b98a76b8bc867ae957bd786fba7a50654c0d
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

comment:31 Changed 3 years ago by
 Commit changed from ddf4b98a76b8bc867ae957bd786fba7a50654c0d to eff01197b87fba2ff81f85de4351ee67df8cbe79
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

comment:32 Changed 3 years ago by
 Dependencies changed from #24814, #24828, #24829 to #24814, #24828, #24829, #24863
comment:33 Changed 3 years ago by
 Dependencies changed from #24814, #24828, #24829, #24863 to #24814, #24828, #24829, #24863, #24865
comment:34 Changed 3 years ago by
 Commit changed from eff01197b87fba2ff81f85de4351ee67df8cbe79 to 390e49e328296309a4a4736f97d49c6a8cbc4e43
comment:35 Changed 3 years ago by
 Commit changed from 390e49e328296309a4a4736f97d49c6a8cbc4e43 to ce371891a6d80333ed4e502b472d8597230ca031
comment:36 Changed 3 years ago by
 Commit changed from ce371891a6d80333ed4e502b472d8597230ca031 to b024a7ebfa89bac922c129f29139b3265212c75a
comment:37 Changed 3 years ago by
 Commit changed from b024a7ebfa89bac922c129f29139b3265212c75a to 6bdfb8112bfbf99b6368e7268eb548761ffea2fe
comment:38 Changed 3 years ago by
 Dependencies changed from #24814, #24828, #24829, #24863, #24865 to #24814, #24828, #24829, #24863, #24865, #24874
comment:39 Changed 3 years ago by
 Commit changed from 6bdfb8112bfbf99b6368e7268eb548761ffea2fe to 4944cd47e704c58554fde0a198084651266cdc8c
comment:40 Changed 3 years ago by
 Commit changed from 4944cd47e704c58554fde0a198084651266cdc8c to 4ef5dc20076d91fdd84fd6ee6a37e50d258a4ee9
comment:41 Changed 3 years ago by
 Description modified (diff)
comment:42 Changed 3 years ago by
 Commit changed from 4ef5dc20076d91fdd84fd6ee6a37e50d258a4ee9 to d5c9ee0593755e3effad20b02b8f920ea2c799f3
comment:43 Changed 3 years ago by
 Commit changed from d5c9ee0593755e3effad20b02b8f920ea2c799f3 to 3f722cfe6b66631a20e9872d3a3db2515849a670
comment:44 Changed 3 years ago by
 Dependencies changed from #24814, #24828, #24829, #24863, #24865, #24874 to #24814, #24828, #24829, #24863, #24865, #24874, #24881
comment:45 Changed 3 years ago by
 Commit changed from 3f722cfe6b66631a20e9872d3a3db2515849a670 to f190e72c0c8a487469a41321dcff31a01f733c82
comment:46 Changed 3 years ago by
 Dependencies changed from #24814, #24828, #24829, #24863, #24865, #24874, #24881 to #24814, #24828, #24829, #24863, #24865, #24874, #24881, #24884
comment:47 Changed 3 years ago by
 Commit changed from f190e72c0c8a487469a41321dcff31a01f733c82 to b9ee50f3dd0b20ae8bd26606ad63363b05033350
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

comment:48 Changed 3 years ago by
 Description modified (diff)
 Status changed from new to needs_review
comment:49 Changed 3 years ago by
 Commit changed from b9ee50f3dd0b20ae8bd26606ad63363b05033350 to 0cc0d5d2392ae97ebb0a28608a0c8494a3f0f64a
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

comment:50 Changed 3 years ago by
 Dependencies changed from #24814, #24828, #24829, #24863, #24865, #24874, #24881, #24884 to #24828, #24863, #24865, #24874, #24881, #24884
comment:51 Changed 3 years ago by
 Commit changed from 0cc0d5d2392ae97ebb0a28608a0c8494a3f0f64a to 850a2fae83a3b15edae004ac85695130c76a31b3
comment:52 Changed 3 years ago by
 Commit changed from 850a2fae83a3b15edae004ac85695130c76a31b3 to f5a7ddbabed7da0e983e80250073d7dfcf56b601
comment:53 Changed 3 years ago by
 Commit changed from f5a7ddbabed7da0e983e80250073d7dfcf56b601 to 1ba4b76970b0f3d63404a7c4f28d026f08e7bcec
comment:54 Changed 3 years ago by
 Dependencies changed from #24828, #24863, #24865, #24874, #24881, #24884 to #24828, #24945
comment:55 followup: ↓ 61 Changed 3 years ago by
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 3 years ago by
 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 3 years ago by
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 3 years ago by
The slowdown within timeit()
is tracked at #24954.
comment:59 Changed 3 years ago by
 Commit changed from 1ba4b76970b0f3d63404a7c4f28d026f08e7bcec to fac3f5e2aec876cfae4ec8c0bc18879a19cd001d
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 lengthchecking iterator

fac3f5e  New MatrixArgs object to deal with constructing matrices

comment:60 Changed 3 years ago by
 Dependencies changed from #24828, #24945 to #24828, #24947, #24945
 Status changed from needs_work to needs_review
comment:61 in reply to: ↑ 55 Changed 3 years ago by
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 2and
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 3 years ago by
 Reviewers set to Marc Mezzarobba
 Status changed from needs_review to positive_review
comment:63 Changed 3 years ago by
 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:
bf9cefd  New MatrixArgs object to deal with constructing matrices

comment:64 Changed 3 years ago by
 Dependencies #24828, #24947, #24945 deleted
 Description modified (diff)
 Status changed from needs_review to positive_review
Trivially rebased.
comment:65 Changed 3 years ago by
 Description modified (diff)
comment:66 Changed 3 years ago by
 Description modified (diff)
comment:67 followup: ↓ 68 Changed 3 years ago by
 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 3 years ago by
comment:69 Changed 3 years ago by
 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 3 years ago by
 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 3 years ago by
With the updated version of #24370, everything works fine! So both tickets can get positive review.
comment:72 Changed 3 years ago by
 Status changed from needs_work to positive_review
comment:73 Changed 3 years ago by
 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
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
New MatrixInput object to deal with constructing matrices