Sage: Ticket #10628: initialization of matrices from vectors or list of lists can be way faster
https://trac.sagemath.org/ticket/10628
<p>
This is demonstrated by the code below:
</p>
<pre class="wiki">sage: M=MatrixSpace(GF(46337),400)
sage: m=M.random_element()
sage: m1=m.columns()
sage: time M(m1)
CPU times: user 3.06 s, sys: 0.44 s, total: 3.49 s
Wall time: 3.70 s
400 x 400 dense matrix over Finite Field of size 46337
sage: x=[]
sage: time map(x.extend,m1)
CPU times: user 0.81 s, sys: 0.02 s, total: 0.83 s
sage: time M(x)
CPU times: user 0.01 s, sys: 0.00 s, total: 0.01 s
Wall time: 0.01 s
400 x 400 dense matrix over Finite Field of size 46337
sage: M(x)==M(m1)
True
</pre><p>
Apply:
</p>
<ol><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/10628/10628-speedup-matrix-creation" title="Attachment '10628-speedup-matrix-creation' in Ticket #10628">10628-speedup-matrix-creation</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/10628/10628-speedup-matrix-creation" title="Download"></a>
</li><li><a class="attachment" href="https://trac.sagemath.org/attachment/ticket/10628/10628-stacked-matrix-creation.patch" title="Attachment '10628-stacked-matrix-creation.patch' in Ticket #10628">10628-stacked-matrix-creation.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/10628/10628-stacked-matrix-creation.patch" title="Download"></a>
</li></ol>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/10628
Trac 1.1.6mderickxThu, 13 Jan 2011 22:48:51 GMTstatus changed
https://trac.sagemath.org/ticket/10628#comment:1
https://trac.sagemath.org/ticket/10628#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
<p>
timing with the new code:
</p>
<pre class="wiki">sage: M=MatrixSpace(GF(46337),400)
sage: m=M.random_element()
sage: m1=m.columns()
sage: time M(m1)
CPU times: user 0.83 s, sys: 0.06 s, total: 0.88 s
Wall time: 1.03 s
400 x 400 dense matrix over Finite Field of size 46337
</pre>
TicketrbeezerFri, 21 Jan 2011 05:37:46 GMT
https://trac.sagemath.org/ticket/10628#comment:2
https://trac.sagemath.org/ticket/10628#comment:2
<p>
Based on a quick look - two questions and a suggestion.
</p>
<ol><li> Does this address the complaint at <a class="new ticket" href="https://trac.sagemath.org/ticket/10312" title="defect: very slow matrix construction or block_matrix (new)">#10312</a>? Fully, or partially?
</li></ol><ol start="2"><li> There is a command <code>is_Vector()</code>. Or do we just care that <code>x[0]</code> (and the rest of x) is made up of iterables? Would anything that can list itself qualify? Might there be a better test?
</li></ol><p>
If you name your patch so it ends in ".patch" then Trac will give a nice red/green view of it in a web browser. Without, its just the raw patch test.
</p>
<p>
Thanks,
Rob
</p>
TicketmderickxSat, 22 Jan 2011 10:27:54 GMT
https://trac.sagemath.org/ticket/10628#comment:3
https://trac.sagemath.org/ticket/10628#comment:3
<ol><li>It adresses the complaint at 10312 partially, it's not as fast as their suggestion. But at least a lot faster.
<pre class="wiki">0 0.000539 0.000325
50 0.031196 0.026185
100 0.065819 0.051949
150 0.199419 0.080579
200 0.244128 0.108761
250 0.389423 0.137844
300 0.337158 0.165608
350 0.473639 0.194574
400 0.521386 0.220917
450 0.676239 0.249057
500 0.620335 0.277434
550 0.755271 0.307235
600 0.942296 0.333725
650 0.881207 0.364136
700 1.050021 0.393484
750 1.101895 0.4192
800 1.143609 0.447968
850 1.348676 0.476717
900 1.244891 0.504382
950 1.557877 0.534018
1000 1.497431 0.572604}}}
Didn't look at sparse matrices.
2. I didn't think about the testing, just left it as it was, since I just wanted the code to be faster, not to behave in different way. If you have suggestions for better testing I'm willing to add it to the patch, but still not feeling about figuring out the best way to test myself.
</pre></li></ol>
TicketmderickxSat, 22 Jan 2011 10:32:11 GMT
https://trac.sagemath.org/ticket/10628#comment:4
https://trac.sagemath.org/ticket/10628#comment:4
<p>
By the way, the best way to do solve these tickets is probably to write a cython "matrix_from_list_of_iterables" command and call that from the vector initialization, but I don't know if that would be worth it since most methods done on matrices will still be way slower than initializing them. I.e. this patch makes sure that initializing is no longer the bottle neck.
</p>
TicketmderickxSat, 22 Jan 2011 11:49:52 GMTcc set
https://trac.sagemath.org/ticket/10628#comment:5
https://trac.sagemath.org/ticket/10628#comment:5
<ul>
<li><strong>cc</strong>
<em>rbeezer</em> added
</li>
</ul>
TicketrbeezerSun, 23 Jan 2011 04:21:12 GMT
https://trac.sagemath.org/ticket/10628#comment:6
https://trac.sagemath.org/ticket/10628#comment:6
<p>
Well, the best thing to do would be to rewrite the entire matrix construction routines. ;-) I've made lots of changes to the vector constructor recently, but the matrix constructor always looks like a much bigger job.
</p>
<p>
The <code>is_vector</code> attribute check is pretty bad - an object could have that method and the method could return <code>False</code>. But I can understand the desire to just change small parts (which is fine). That said, is it safe to remove <code>copy = False</code>?
</p>
<p>
I'll run some tests overnight.
</p>
<p>
Rob
</p>
TicketrbeezerWed, 26 Jan 2011 05:00:47 GMT
https://trac.sagemath.org/ticket/10628#comment:7
https://trac.sagemath.org/ticket/10628#comment:7
<p>
Tests ran fine. Even so, I can't see that it is safe to drop the <code>Copy = False</code> line. It could be, I just can't chase my way through to be sure.
</p>
<p>
I'd feel a lot better about giving this a positive review if it were still there.
</p>
<p>
Rob
</p>
TicketmderickxWed, 26 Jan 2011 15:38:26 GMT
https://trac.sagemath.org/ticket/10628#comment:8
https://trac.sagemath.org/ticket/10628#comment:8
<p>
Since I combined the three different cases into one I thought it was saver to drop the Copy = False line. Making a copy when not really needed is saver then the other way around, it does increase the peak memory usage and the runtime tough. Since in the for loop we already create an entirely new list it will be safe to add copy=False for all three cases. If you think that change is ok I will add it. Else I will just add the copy=false in the vector case.
</p>
TicketSimonKingWed, 27 Jul 2011 16:54:01 GMTstatus changed
https://trac.sagemath.org/ticket/10628#comment:9
https://trac.sagemath.org/ticket/10628#comment:9
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
If you look at the init method of <code>Matrix_generic_dense</code>, you see that the given list of entries is simply assigned to the attribute <code>self._entries</code>, if copy is False, and a copy of the list of entries is assigned to <code>self._entries</code> otherwise.
</p>
<p>
Note that the init method of <code>Matrix_generic_dense</code> actually does not use the "copy" function: The copy is constructed explicitly. I could imagine that this is not optimal, but that's a different topic.
</p>
<p>
Now, is it safe to keep "copy=False" in your patch? I think so. You create a new list, both when you transform a list of vectors resp. a list of lists into a single list of entries. So, "copy=True" would be a waste of time.
</p>
<p>
Concerning the test for the presence of "is_vector": I think one could easily do better.
</p>
<p>
Whithout your patch, what one actually used was the method "list()" of vectors (see line 28 of your patch). With your patch, you use that a vector is iterable, by doing e.extend(v) in line 30 of your patch.
</p>
<p>
If I am not mistaken, good Python tradition is to duck test using the methods that are actually needed. Hence, without your patch, it would have been better to simply try to use "v.list()", and catch the attribute error. And with your patch, it would be better to simply try "e.extend(v)", and catch the error that results if v is not iterable.
</p>
<p>
Moreover, I really think that the following should be made work:
</p>
<pre class="wiki">sage: MS = MatrixSpace(GF(5),4,2)
sage: MS0 = MatrixSpace(GF(5),2)
sage: MS([MS0.random_element(), MS0.random_element()])
</pre><p>
Hence, the matrix constructor should also accept lists of matrices (not just list of vectors), and should automatically stack the matrices from the list.
</p>
<p>
The problem with duck typing via "list()" is that we could have matrices over a ring whose elements have a list() method. In that case, we would not want to use list() in the matrix constructor.
</p>
<p>
Perhaps one could proceed as follows:
</p>
<div class="wiki-code"><div class="code"><pre> <span class="k">if</span> <span class="nb">isinstance</span><span class="p">(</span>x<span class="p">,</span> <span class="nb">list</span><span class="p">)</span> <span class="ow">and</span> <span class="nb">len</span><span class="p">(</span>x<span class="p">)</span> <span class="o">></span> <span class="mi">0</span><span class="p">:</span>
<span class="c"># If the list has the expected length, then</span>
<span class="c"># we are likely to have a list of elements of</span>
<span class="c"># the base ring</span>
<span class="k">if</span> <span class="nb">len</span><span class="p">(</span>x<span class="p">)</span> <span class="o"><</span> <span class="bp">self</span><span class="o">.</span>__ncols<span class="o">*</span><span class="bp">self</span><span class="o">.</span>__nrows<span class="p">:</span>
e <span class="o">=</span> <span class="p">[]</span>
<span class="k">for</span> v <span class="ow">in</span> x<span class="p">:</span>
<span class="k">try</span><span class="p">:</span>
e<span class="o">.</span>extend<span class="p">(</span>v<span class="o">.</span>list<span class="p">())</span>
<span class="k">except</span> <span class="ne">AttributeError</span><span class="p">:</span>
e<span class="o">.</span>extend<span class="p">(</span>v<span class="p">)</span>
x <span class="o">=</span> e
<span class="c"># x is a new list, hence, copy=False is OK</span>
copy <span class="o">=</span> <span class="bp">False</span>
<span class="c"># Now, x presumably is a list of ring elements.</span>
<span class="k">if</span> <span class="ow">not</span> rows<span class="p">:</span>
new_x <span class="o">=</span> <span class="p">[]</span>
<span class="k">for</span> k <span class="ow">in</span> <span class="nb">xrange</span><span class="p">(</span><span class="nb">len</span><span class="p">(</span>x<span class="p">)):</span>
i <span class="o">=</span> k <span class="o">%</span> <span class="bp">self</span><span class="o">.</span>__ncols
j <span class="o">=</span> k <span class="o">//</span> <span class="bp">self</span><span class="o">.</span>__ncols
new_x<span class="o">.</span>append<span class="p">(</span> x<span class="p">[</span> i<span class="o">*</span><span class="bp">self</span><span class="o">.</span>__nrows <span class="o">+</span> j <span class="p">]</span> <span class="p">)</span>
x <span class="o">=</span> new_x
<span class="k">return</span> <span class="bp">self</span><span class="o">.</span>__matrix_class<span class="p">(</span><span class="bp">self</span><span class="p">,</span> entries<span class="o">=</span>x<span class="p">,</span> copy<span class="o">=</span>copy<span class="p">,</span> <span class="nb">coerce</span><span class="o">=</span><span class="nb">coerce</span><span class="p">)</span>
</pre></div></div><p>
Do you think that my idea makes sense? It would allow to define a matrix by a list of (lists, matrices, vectors) -- the three could actually be combined.
</p>
<p>
I have to leave office now, but I hope that I'll find time to create a patch later or tomorrow.
</p>
TicketSimonKingThu, 28 Jul 2011 09:40:51 GMTstatus, priority changed; author set
https://trac.sagemath.org/ticket/10628#comment:10
https://trac.sagemath.org/ticket/10628#comment:10
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>priority</strong>
changed from <em>minor</em> to <em>major</em>
</li>
<li><strong>author</strong>
set to <em>Maarten Derickx, Simon King</em>
</li>
</ul>
<p>
I have attached a patch with my suggested code (with some minor modifications) and a new doc test.
</p>
<p>
Also I can provide more timings. They show that the second patch not only provides more flexibility but also a mild speedup on top of what is achieved by the first patch.
</p>
<p>
Here are the timings on my computer for the examples you gave, with both, only the first, and no patch, respectively (I considered the wall time in each case):
</p>
<pre class="wiki">sage: M=MatrixSpace(GF(46337),400)
sage: m=M.random_element()
sage: m1=m.columns()
sage: time M(m1)
400 x 400 dense matrix over Finite Field of size 46337
Time: CPU 0.68 s, Wall: 0.67 s
// without second patch: 0.72 s
// without patches: 1.46 s
sage: M=MatrixSpace(GF(46337),400)
sage: m=M.random_element()
sage: m1=m.columns()
sage: time M(m1)
400 x 400 dense matrix over Finite Field of size 46337
Time: CPU 0.66 s, Wall: 0.66 s
// without second patch: 0.66 s
// without patches: 1.38 s
</pre><p>
The ticket description mentions the creation from list of lists, but there were no timings, so far.
</p>
<pre class="wiki">sage: MS = MatrixSpace(GF(5),1000)
sage: L = 1000*[[GF(5)(i) for i in range(1000)]]
sage: %time M = MS(L)
CPU times: user 0.02 s, sys: 0.00 s, total: 0.02 s
Wall time: 0.03 s
// without second patch: 0.04 s
// without both patches: 5.14 s
sage: sum(L,[]) == M.list()
True
</pre><p>
If a matrix is created from a plain list (not list of lists), the first patch had a slow down. The second patch brings it back to the original speed:
</p>
<pre class="wiki">sage: MS = MatrixSpace(GF(5),4000)
# Matrix creation by a single list
sage: L = 4000*[GF(5)(i) for i in range(4000)]
sage: %time M = MS(L)
CPU times: user 0.20 s, sys: 0.02 s, total: 0.22 s
Wall time: 0.23 s
// without second patch: 0.38 s
// without both patches: 0.21 s
</pre><p>
Here are the new doc tests, that would fail without the second patch:
</p>
<pre class="wiki"> sage: MS = MatrixSpace(ZZ,4,2)
sage: MS0 = MatrixSpace(ZZ,2)
A list of matrices::
sage: MS.matrix([MS0([1,2,3,4]), MS0([5,6,7,8])])
[1 2]
[3 4]
[5 6]
[7 8]
A mixed list of matrices and vectors::
sage: MS.matrix( [MS0([1,2,3,4])] + list(MS0([5,6,7,8])) )
[1 2]
[3 4]
[5 6]
[7 8]
</pre><p>
Last, allow me to increase the priority of the ticket. An improvement from more than 5 seconds to less than 0.05 seconds is not "minor".
</p>
TicketSimonKingThu, 28 Jul 2011 09:42:53 GMT
https://trac.sagemath.org/ticket/10628#comment:11
https://trac.sagemath.org/ticket/10628#comment:11
<p>
Concerning review: I am running full long doctests now (so far, I only did the tests from sage/matrix).
</p>
<p>
As much as I know, cross-reviewing is OK. So, I could give the first patch a review (which will be positive, if all tests pass), and someone else could give my patch a review.
</p>
TicketSimonKingThu, 28 Jul 2011 09:52:11 GMTstatus changed
https://trac.sagemath.org/ticket/10628#comment:12
https://trac.sagemath.org/ticket/10628#comment:12
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Too bad. I got a failure in devel/sage-main/doc/en/bordeaux_2008/elliptic_curves.rst. That failure does not occur with only the first patch.
</p>
<p>
Hence, back to the drawing board...
</p>
TicketmderickxThu, 28 Jul 2011 11:46:43 GMT
https://trac.sagemath.org/ticket/10628#comment:13
https://trac.sagemath.org/ticket/10628#comment:13
<p>
Cool, thanks for showing interest in this ticket. I will try to look at your work if you have a updated patch witch passes the doctests.
</p>
TicketSimonKingThu, 28 Jul 2011 11:50:53 GMT
https://trac.sagemath.org/ticket/10628#comment:14
https://trac.sagemath.org/ticket/10628#comment:14
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10628#comment:13" title="Comment 13">mderickx</a>:
</p>
<blockquote class="citation">
<p>
Cool, thanks for showing interest in this ticket. I will try to look at your work if you have a updated patch witch passes the doctests.
</p>
</blockquote>
<p>
I see what happened.
</p>
<p>
I test whether the length of x coincides with the number of entries of the matrix. If it does, then it is assumed that it is a list of ring elements.
</p>
<p>
The error occurs if the matrix space has one row and one column. If the input is, say, <code>[[0]]</code>, then the length of that list is one. Hence, with my patch, it is directly forwarded to the init method of the matrix class -- but it <em>should</em> send <code>[0]</code> to the init method.
</p>
<p>
Fixing it now, then repeating the timings...
</p>
TicketSimonKingThu, 28 Jul 2011 12:32:36 GMTmilestone set
https://trac.sagemath.org/ticket/10628#comment:15
https://trac.sagemath.org/ticket/10628#comment:15
<ul>
<li><strong>milestone</strong>
set to <em>sage-4.7.2</em>
</li>
</ul>
<p>
The corner case is really annoying. It occurs if the number of columns is 1 -- because that means that both <code>[1,2,3,...]</code> and <code>[[1],[2],[3],...]</code> are valid input.
</p>
<p>
So, it was wrong that I used the length of the list as indicator of what happens.
</p>
TicketSimonKingThu, 28 Jul 2011 13:20:51 GMT
https://trac.sagemath.org/ticket/10628#comment:16
https://trac.sagemath.org/ticket/10628#comment:16
<p>
There is one more problem with duck typing. Unfortunately, <code>NumberFieldElement_quadratic</code> has the attribute <code>list()</code>. Hence, my patch will systematically fail on matrices over (quadratic) number fields.
</p>
<p>
So, it is more than just a corner case.
</p>
TicketSimonKingThu, 28 Jul 2011 16:20:35 GMTstatus changed
https://trac.sagemath.org/ticket/10628#comment:17
https://trac.sagemath.org/ticket/10628#comment:17
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I updated my patch. Now, the doctests in sage/matrix, sage/schemes/elliptic_curves and doc/ pass.
</p>
<p>
Let me summarise the problem:
</p>
<ul><li>I think it should be possible to create a matrix from a list of vectors <em>or</em> matrices; actually it should be possible to combine both.
</li><li>The old "ducktyping" of vectors is not suitable for matrices, because they don't have a method "is_vector()".
</li><li>Both vectors and matrices have a method "list()", and that is exactly what is used in the pre-processing of data. Hence, why not just duck-type via "list()"?
</li><li>Problem: Number field elements have that method as well. If v is a number field element, then v.list() returns a pair of numbers - that is not what we want.
</li></ul><p>
My new patch works as follows:
</p>
<ol><li>Preprocessing is needed if the input is not a plain list of elements. If the given list is too short or if the number of columns is one, then it is possible that the given list contains something that is not just a ring element (e.g., a vector, a matrix, or a list). That is line 1231 of my patch.
</li><li>Since pure duck typing turned out to be error prone, I test in line 1237 whether we have a list or tuple using "isinstance", in which case we extend our list of entries.
</li><li>Otherwise, duck typing is used to catch vectors and matrices. It turns out that both vectors and matrices have an attribute "row()" and an attribute "list()", whereas number field elements have no "row()". If we have list/vector, then we extend our list of entries by "v.list()".
</li><li>Otherwise, we suppose that we have an element of the base ring (or something that will eventually be converted into the base ring - that case is subject to a new doc test), and append it to our list of entries.
</li></ol><p>
The doc test problems concerning number field elements vanished with the new patch version. Moreover, the speed is still fine. I am running all long doc tests now, and return to "needs review".
</p>
TicketSimonKingThu, 28 Jul 2011 18:09:23 GMTdescription changed; reviewer set
https://trac.sagemath.org/ticket/10628#comment:18
https://trac.sagemath.org/ticket/10628#comment:18
<ul>
<li><strong>reviewer</strong>
set to <em>Simon King</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10628?action=diff&version=18">diff</a>)
</li>
</ul>
<p>
All long tests both in devel/sage/sage and devel/sage/doc pass. From that point of view, and since it brings quite a speed-up, I can give the first patch a positive review. I hope that someone else can review my patch as well.
</p>
TicketSimonKingMon, 15 Aug 2011 09:37:26 GMT
https://trac.sagemath.org/ticket/10628#comment:19
https://trac.sagemath.org/ticket/10628#comment:19
<p>
We have two patches. Maarten's patch got a positive review from me. My patch still needs a review from someone (most naturally from Maarten...).
</p>
TicketSimonKingFri, 26 Aug 2011 06:53:10 GMT
https://trac.sagemath.org/ticket/10628#comment:20
https://trac.sagemath.org/ticket/10628#comment:20
<p>
One of the patches still needs a review, even though sage days is now over...
</p>
TicketmderickxThu, 01 Sep 2011 12:27:01 GMTstatus changed
https://trac.sagemath.org/ticket/10628#comment:21
https://trac.sagemath.org/ticket/10628#comment:21
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
I wanted to start and see if all doctest pass, but it needs to be rebased first. I will do this, this evening.
</p>
<pre class="wiki">mderickx@sage:/mnt/usb1/scratch/mderickx/sage-4.7.2.alpha2/devel/sage$ sage -hg qim -P http://trac.sagemath.org/sage_trac/raw-attachment/ticket/10628/10628-speedup-matrix-creation
adding 10628-speedup-matrix-creation to series file
applying 10628-speedup-matrix-creation
patching file sage/matrix/matrix_space.py
Hunk #2 succeeded at 1225 with fuzz 2 (offset 106 lines).
now at: 10628-speedup-matrix-creation
mderickx@sage:/mnt/usb1/scratch/mderickx/sage-4.7.2.alpha2/devel/sage$ sage -hg qim -P http://trac.sagemath.org/sage_trac/raw-attachment/ticket/10628/10628-stacked-matrix-creation.patch
adding 10628-stacked-matrix-creation.patch to series file
applying 10628-stacked-matrix-creation.patch
patching file sage/matrix/matrix_space.py
Hunk #1 FAILED at 1160
Hunk #2 FAILED at 1177
Hunk #3 FAILED at 1189
3 out of 3 hunks FAILED -- saving rejects to file sage/matrix/matrix_space.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 10628-stacked-matrix-creation.patch
</pre>
TicketSimonKingFri, 28 Oct 2011 14:50:11 GMTattachment set
https://trac.sagemath.org/ticket/10628
https://trac.sagemath.org/ticket/10628
<ul>
<li><strong>attachment</strong>
set to <em>10628-stacked-matrix-creation.patch</em>
</li>
</ul>
<p>
Allow mixed lists of lists/vectors/matrices in matrix creation
</p>
TicketSimonKingFri, 28 Oct 2011 14:51:02 GMTstatus changed
https://trac.sagemath.org/ticket/10628#comment:22
https://trac.sagemath.org/ticket/10628#comment:22
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I have replaced the second patch. I hope that it now applies cleanly, so that the review can finally be finished!
</p>
TicketjasonTue, 13 Dec 2011 16:50:53 GMTcc changed
https://trac.sagemath.org/ticket/10628#comment:23
https://trac.sagemath.org/ticket/10628#comment:23
<ul>
<li><strong>cc</strong>
<em>jason</em> added
</li>
</ul>
TicketmderickxSat, 17 Dec 2011 22:12:51 GMT
https://trac.sagemath.org/ticket/10628#comment:24
https://trac.sagemath.org/ticket/10628#comment:24
<p>
I just ran doctestst and
</p>
<pre class="wiki">----------------------------------------------------------------------
All tests passed!
Total time for all tests: 667.4 seconds
</pre>
TicketmderickxThu, 22 Dec 2011 22:59:42 GMT
https://trac.sagemath.org/ticket/10628#comment:25
https://trac.sagemath.org/ticket/10628#comment:25
<p>
I've read trough to code and everything looks sensible to me. There was a problem with reproducing the slowdown that my patch introces. On sage.math.washington.edu (with sage.4.8.alpha4) I get something between 0.30 and 0.33 seconds for the following test:
</p>
<pre class="wiki">sage: MS = MatrixSpace(GF(5),4000)
# Matrix creation by a single list
sage: L = 4000*[GF(5)(i) for i in range(4000)]
sage: %time M = MS(L)
</pre><p>
And that timing stays the same with either my, or both patches applied.
</p>
<p>
But since the timing is only not reproducible for my patch, this ticket can still have positive review.
</p>
TicketmderickxMon, 30 Jan 2012 17:05:22 GMTattachment set
https://trac.sagemath.org/ticket/10628
https://trac.sagemath.org/ticket/10628
<ul>
<li><strong>attachment</strong>
set to <em>10628-speedup-matrix-creation</em>
</li>
</ul>
TicketmderickxMon, 30 Jan 2012 17:09:37 GMTstatus, description changed
https://trac.sagemath.org/ticket/10628#comment:26
https://trac.sagemath.org/ticket/10628#comment:26
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10628?action=diff&version=26">diff</a>)
</li>
</ul>
<p>
I noticed that I didn't actually put it to positive review. I tested if the patches applied to the new 5.0.beta1, and they did so with fuzz. I rebased them and they now apply without fuzz, all tests still pass hence now really a positive review.
</p>
TicketjdemeyerMon, 06 Feb 2012 21:23:20 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/10628#comment:27
https://trac.sagemath.org/ticket/10628#comment:27
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.0.beta3</em>
</li>
</ul>
Ticket