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

Priority:  major  Milestone:  sage8.2 
Component:  linear algebra  Keywords:  
Cc:  Travis Scrimshaw, Vincent Delecroix, Erik Bray  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 5 years ago by
Cc:  Travis Scrimshaw Vincent Delecroix added 

Description:  modified (diff) 
comment:2 Changed 5 years ago by
Description:  modified (diff) 

comment:3 Changed 5 years ago by
Branch:  → u/jdemeyer/new_matrixinput_object_to_deal_with_constructing_matrices 

comment:4 Changed 5 years ago by
Commit:  → de8775b89e49cccf4893ef515b50eea3afd9d375 

comment:5 Changed 5 years ago by
Commit:  de8775b89e49cccf4893ef515b50eea3afd9d375 → 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 5 years ago by
Commit:  e02c31aebd10b85b016bbcf1536330c59685fa29 → 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 5 years ago by
Commit:  399b2bbee6c63b71c537087a8af1d59e80890947 → 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 5 years ago by
Commit:  7f18749fd1c45eab5b3c2ca2633233d76e2d7e6a → 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 5 years ago by
Commit:  83c09c35b606df47b451bbfe938bc79148220e38 → 6d91682a1c80f7f83ea86e3b6018ae2a5e4ab5b5 

comment:10 Changed 5 years ago by
Summary:  New MatrixInput object to deal with constructing matrices → New MatrixArgs object to deal with constructing matrices 

comment:11 Changed 5 years ago by
Description:  modified (diff) 

comment:12 Changed 5 years ago by
Commit:  6d91682a1c80f7f83ea86e3b6018ae2a5e4ab5b5 → 1ed6a49b23be9a7fd4a25cbcaa98b178ba49756f 

comment:13 Changed 5 years ago by
Commit:  1ed6a49b23be9a7fd4a25cbcaa98b178ba49756f → 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 5 years ago by
Commit:  0ce108f00fd94b71a8e384c98c9e96db30eaae92 → 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 5 years ago by
Commit:  ddff20fd1b009e38b9337031882c9c89d362fc62 → 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 5 years ago by
Description:  modified (diff) 

comment:17 Changed 5 years ago by
Cc:  Erik Bray added 

Haven't ready it 100% yet but I get the idea. Definitely looks like an improvement.
comment:18 Changed 5 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 5 years ago by
Commit:  ec0c619abda3a2fe16c6cd3e36c0b38210ae59d2 → a0f37724d92c0810c4b37c00625e9e404d728de3 

comment:20 Changed 5 years ago by
Dependencies:  → #24814 

comment:21 Changed 5 years ago by
Commit:  a0f37724d92c0810c4b37c00625e9e404d728de3 → 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 5 years ago by
Commit:  769af020c25a7dc4f997289e8fdc759a0469c02a → 6b8aed036ea1e844ae9b6716ec180dde29a873e4 

comment:23 Changed 5 years ago by
Commit:  6b8aed036ea1e844ae9b6716ec180dde29a873e4 → 9a1d7b0fbdfd9ed6b20db7e62433d797b953ed33 

comment:24 Changed 5 years ago by
Dependencies:  #24814 → #24814, #24828 

comment:25 Changed 5 years ago by
Commit:  9a1d7b0fbdfd9ed6b20db7e62433d797b953ed33 → 3b77656e2a0ced9b6a25fd837bd1ace8ee47cf0e 

comment:26 Changed 5 years ago by
Dependencies:  #24814, #24828 → #24814, #24828, #24829 

comment:27 Changed 5 years ago by
Commit:  3b77656e2a0ced9b6a25fd837bd1ace8ee47cf0e → 51105fb09a9daaef4aa82f32b97d3347d655f83c 

comment:28 Changed 5 years ago by
Commit:  51105fb09a9daaef4aa82f32b97d3347d655f83c → 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 5 years ago by
Commit:  333173b4fe37a049d835bb7353b9d124045f99cf → a7146ce1f3ca556bb9ded20e0c7c0a04ace0f40a 

comment:30 Changed 5 years ago by
Commit:  a7146ce1f3ca556bb9ded20e0c7c0a04ace0f40a → 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 5 years ago by
Commit:  ddf4b98a76b8bc867ae957bd786fba7a50654c0d → 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 5 years ago by
Dependencies:  #24814, #24828, #24829 → #24814, #24828, #24829, #24863 

comment:33 Changed 5 years ago by
Dependencies:  #24814, #24828, #24829, #24863 → #24814, #24828, #24829, #24863, #24865 

comment:34 Changed 5 years ago by
Commit:  eff01197b87fba2ff81f85de4351ee67df8cbe79 → 390e49e328296309a4a4736f97d49c6a8cbc4e43 

comment:35 Changed 5 years ago by
Commit:  390e49e328296309a4a4736f97d49c6a8cbc4e43 → ce371891a6d80333ed4e502b472d8597230ca031 

comment:36 Changed 5 years ago by
Commit:  ce371891a6d80333ed4e502b472d8597230ca031 → b024a7ebfa89bac922c129f29139b3265212c75a 

comment:37 Changed 5 years ago by
Commit:  b024a7ebfa89bac922c129f29139b3265212c75a → 6bdfb8112bfbf99b6368e7268eb548761ffea2fe 

comment:38 Changed 5 years ago by
Dependencies:  #24814, #24828, #24829, #24863, #24865 → #24814, #24828, #24829, #24863, #24865, #24874 

comment:39 Changed 5 years ago by
Commit:  6bdfb8112bfbf99b6368e7268eb548761ffea2fe → 4944cd47e704c58554fde0a198084651266cdc8c 

comment:40 Changed 5 years ago by
Commit:  4944cd47e704c58554fde0a198084651266cdc8c → 4ef5dc20076d91fdd84fd6ee6a37e50d258a4ee9 

comment:41 Changed 5 years ago by
Description:  modified (diff) 

comment:42 Changed 5 years ago by
Commit:  4ef5dc20076d91fdd84fd6ee6a37e50d258a4ee9 → d5c9ee0593755e3effad20b02b8f920ea2c799f3 

comment:43 Changed 5 years ago by
Commit:  d5c9ee0593755e3effad20b02b8f920ea2c799f3 → 3f722cfe6b66631a20e9872d3a3db2515849a670 

comment:44 Changed 5 years ago by
Dependencies:  #24814, #24828, #24829, #24863, #24865, #24874 → #24814, #24828, #24829, #24863, #24865, #24874, #24881 

comment:45 Changed 5 years ago by
Commit:  3f722cfe6b66631a20e9872d3a3db2515849a670 → f190e72c0c8a487469a41321dcff31a01f733c82 

comment:46 Changed 5 years ago by
Dependencies:  #24814, #24828, #24829, #24863, #24865, #24874, #24881 → #24814, #24828, #24829, #24863, #24865, #24874, #24881, #24884 

comment:47 Changed 5 years ago by
Commit:  f190e72c0c8a487469a41321dcff31a01f733c82 → 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 5 years ago by
Description:  modified (diff) 

Status:  new → needs_review 
comment:49 Changed 5 years ago by
Commit:  b9ee50f3dd0b20ae8bd26606ad63363b05033350 → 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 5 years ago by
Dependencies:  #24814, #24828, #24829, #24863, #24865, #24874, #24881, #24884 → #24828, #24863, #24865, #24874, #24881, #24884 

comment:51 Changed 5 years ago by
Commit:  0cc0d5d2392ae97ebb0a28608a0c8494a3f0f64a → 850a2fae83a3b15edae004ac85695130c76a31b3 

comment:52 Changed 5 years ago by
Commit:  850a2fae83a3b15edae004ac85695130c76a31b3 → f5a7ddbabed7da0e983e80250073d7dfcf56b601 

comment:53 Changed 5 years ago by
Commit:  f5a7ddbabed7da0e983e80250073d7dfcf56b601 → 1ba4b76970b0f3d63404a7c4f28d026f08e7bcec 

comment:54 Changed 5 years ago by
Dependencies:  #24828, #24863, #24865, #24874, #24881, #24884 → #24828, #24945 

comment:55 followup: 61 Changed 5 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 5 years ago by
Status:  needs_review → 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 5 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:59 Changed 5 years ago by
Commit:  1ba4b76970b0f3d63404a7c4f28d026f08e7bcec → 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 5 years ago by
Dependencies:  #24828, #24945 → #24828, #24947, #24945 

Status:  needs_work → needs_review 
comment:61 Changed 5 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 5 years ago by
Reviewers:  → Marc Mezzarobba 

Status:  needs_review → positive_review 
comment:63 Changed 5 years ago by
Commit:  fac3f5e2aec876cfae4ec8c0bc18879a19cd001d → bf9cefd138bb9fbb19c3c43a778ab230bf962902 

Status:  positive_review → 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 5 years ago by
Dependencies:  #24828, #24947, #24945 

Description:  modified (diff) 
Status:  needs_review → positive_review 
Trivially rebased.
comment:65 Changed 5 years ago by
Description:  modified (diff) 

comment:66 Changed 5 years ago by
Description:  modified (diff) 

comment:67 followup: 68 Changed 5 years ago by
Status:  positive_review → needs_work 

Failures in src/sage/schemes/riemann_surfaces/riemann_surface.py, possibly due to 24370
comment:68 Changed 5 years ago by
comment:69 Changed 5 years ago by
Status:  needs_work → 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 5 years ago by
Status:  positive_review → needs_work 

Apparently a conflict with #24370. These tickets just need to be calibrated. The fix is clear either way.
comment:71 Changed 5 years ago by
With the updated version of #24370, everything works fine! So both tickets can get positive review.
comment:72 Changed 5 years ago by
Status:  needs_work → positive_review 

comment:73 Changed 5 years ago by
Branch:  u/jdemeyer/new_matrixinput_object_to_deal_with_constructing_matrices → bf9cefd138bb9fbb19c3c43a778ab230bf962902 

Resolution:  → fixed 
Status:  positive_review → 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