Sage: Ticket #12020: bug in zero_matrix rewrite in matrix_space.py
https://trac.sagemath.org/ticket/12020
<p>
Observe this:
</p>
<pre class="wiki">sage: D=DirichletGroup(20);
sage: g=D[7].extend(400); #order 4 character
sage: M=ModularSymbols(g,2,sign=1).cuspidal_subspace()
sage: N=M.new_subspace()
TypeError Traceback (most recent call last)
/mnt/usb1/scratch/wstein/ref/sage-4.8.alpha2-sage.math.washington.edu-x86_64-Linux/local/lib/python2.6/site-packages/sage/rings/number_field/number_field.pyc in _coerce_non_number_field_element_in(self, x)
5399 except (TypeError, AttributeError), msg:
5400 pass
-> 5401 raise TypeError, type(x)
5402
5403 def _coerce_from_str(self, x):
TypeError: <type 'NoneType'>
</pre><p>
Debugging we see that the problem is that this code in matrix_space.py:
</p>
<pre class="wiki"> 479 if entries is None or entries == 0:
480 if self._copy_zero: # faster to copy than to create a new one.
481 return self.zero_matrix().__copy__()
482 else:
--> 483 return self.__matrix_class(self, None, coerce=coerce, copy=copy)
</pre><p>
That None in the last line is then coerced for some reason into a number field (Cyclotomic Field of order 4 and degree) which makes no sense now...
</p>
<p>
This bug was introduced in <a class="closed ticket" href="https://trac.sagemath.org/ticket/11589" title="enhancement: faster zero matrix creation (closed: fixed)">#11589</a>.
</p>
<p>
<strong>Apply</strong> <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/12020/trac_12020_alt.patch" title="Attachment 'trac_12020_alt.patch' in Ticket #12020">trac_12020_alt.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/12020/trac_12020_alt.patch" title="Download"></a> and <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/12020/12020_review.patch" title="Attachment '12020_review.patch' in Ticket #12020">12020_review.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/12020/12020_review.patch" title="Download"></a>
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/12020
Trac 1.1.6wasSun, 13 Nov 2011 19:45:24 GMT
https://trac.sagemath.org/ticket/12020#comment:1
https://trac.sagemath.org/ticket/12020#comment:1
<p>
I figured this out. This bug was introduced by <a class="closed ticket" href="https://trac.sagemath.org/ticket/11589" title="enhancement: faster zero matrix creation (closed: fixed)">#11589</a> (by Simon King and Martin Albrecht). Basically the zero_matrix() method's code was "mangled/changed" (?) when being copied into the <span class="underline">init</span> method by replacing "0" by "None". This breaks the code, since there is no reason that None be coercible into a ring R, and I don't think that has to be supported because, e.g., in Python <code>int(None)</code> is an error. Also, this is a weird change since the zero_matrix method in the new code still uses 0.
</p>
<p>
Patch coming up.
</p>
TicketwasSun, 13 Nov 2011 19:49:37 GMT
https://trac.sagemath.org/ticket/12020#comment:2
https://trac.sagemath.org/ticket/12020#comment:2
<p>
Here is a simple test to reveal the bug. Small size matrices don't show the bug, probably because of some arbitrary caching parameter in <a class="closed ticket" href="https://trac.sagemath.org/ticket/11589" title="enhancement: faster zero matrix creation (closed: fixed)">#11589</a>, which is probably why this bug slipped through our test framework (we *really* need a random larger testing framework!):
</p>
<pre class="wiki">sage: A = MatrixSpace(CyclotomicField(4),60,30)(0)
sage: A.augment(A)
boom!
</pre>
TicketwasSun, 13 Nov 2011 19:52:57 GMTsummary changed
https://trac.sagemath.org/ticket/12020#comment:3
https://trac.sagemath.org/ticket/12020#comment:3
<ul>
<li><strong>summary</strong>
changed from <em>major (BLOCKER!) bug probably in matrix_space.py</em> to <em>bug in zero_matrix rewrite in matrix_space.py</em>
</li>
</ul>
TicketwasSun, 13 Nov 2011 19:53:54 GMTattachment set
https://trac.sagemath.org/ticket/12020
https://trac.sagemath.org/ticket/12020
<ul>
<li><strong>attachment</strong>
set to <em>trac_12020.patch</em>
</li>
</ul>
TicketjdemeyerMon, 14 Nov 2011 08:44:08 GMTattachment set
https://trac.sagemath.org/ticket/12020
https://trac.sagemath.org/ticket/12020
<ul>
<li><strong>attachment</strong>
set to <em>trac_12020.2.patch</em>
</li>
</ul>
<p>
Same patch with corrected doc formatting
</p>
TicketjdemeyerMon, 14 Nov 2011 08:46:29 GMTstatus, description changed; author set
https://trac.sagemath.org/ticket/12020#comment:4
https://trac.sagemath.org/ticket/12020#comment:4
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/12020?action=diff&version=4">diff</a>)
</li>
<li><strong>author</strong>
set to <em>William Stein</em>
</li>
</ul>
TicketmalbMon, 14 Nov 2011 12:20:33 GMT
https://trac.sagemath.org/ticket/12020#comment:5
https://trac.sagemath.org/ticket/12020#comment:5
<p>
I have attached an alternative solution: instead of switching back to <code>0</code> instead of <code>None</code> make <code>Matrix_cyclo_dense</code> deal with <code>None</code> properly. The reason we switched to <code>None</code> was because this is faster since most (?, perhaps only some) matrix classes do nothing if <code>None</code> is passed, while they write 0 along the main diagonal if zero is passed. Of course, one could also fix those matrix classes and switch back to <code>0</code>.
</p>
TicketjdemeyerMon, 14 Nov 2011 14:00:56 GMT
https://trac.sagemath.org/ticket/12020#comment:6
https://trac.sagemath.org/ticket/12020#comment:6
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12020#comment:5" title="Comment 5">malb</a>:
</p>
<blockquote class="citation">
<p>
Of course, one could also fix those matrix classes and switch back to <code>0</code>.
</p>
</blockquote>
<p>
I slightly prefer that because it seems safer to me.
</p>
TicketvbraunMon, 14 Nov 2011 15:08:40 GMT
https://trac.sagemath.org/ticket/12020#comment:7
https://trac.sagemath.org/ticket/12020#comment:7
<p>
I found and fixed this bug earlier in <a class="closed ticket" href="https://trac.sagemath.org/ticket/12000" title="defect: block_matrix over cyclotomics fails (closed: duplicate)">#12000</a>. Of course nobody ever reviews my patches, sniff.
</p>
TicketjdemeyerMon, 14 Nov 2011 15:49:15 GMTmilestone changed
https://trac.sagemath.org/ticket/12020#comment:8
https://trac.sagemath.org/ticket/12020#comment:8
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.8</em> to <em>sage-duplicate/invalid/wontfix</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12020#comment:7" title="Comment 7">vbraun</a>:
</p>
<blockquote class="citation">
<p>
I found and fixed this bug earlier in <a class="closed ticket" href="https://trac.sagemath.org/ticket/12000" title="defect: block_matrix over cyclotomics fails (closed: duplicate)">#12000</a>. Of course nobody ever reviews my patches, sniff.
</p>
</blockquote>
<p>
Especially with such a nice ticket number...
</p>
<p>
Proposing to close this as "duplicate"
</p>
TicketmalbMon, 14 Nov 2011 15:52:49 GMT
https://trac.sagemath.org/ticket/12020#comment:9
https://trac.sagemath.org/ticket/12020#comment:9
<p>
If "this" is "this ticket" then I agree.
</p>
TicketwasMon, 14 Nov 2011 16:18:07 GMT
https://trac.sagemath.org/ticket/12020#comment:10
https://trac.sagemath.org/ticket/12020#comment:10
<p>
The fix for <a class="closed ticket" href="https://trac.sagemath.org/ticket/12000" title="defect: block_matrix over cyclotomics fails (closed: duplicate)">#12000</a> also works for this particular example. I really hope that all other matrix types (now and in the future) respect this None convention, since they will all have the same problem if we go with the <a class="closed ticket" href="https://trac.sagemath.org/ticket/12000" title="defect: block_matrix over cyclotomics fails (closed: duplicate)">#12000</a> fix rather than the fix here. We could add documentation in the file matrix/docs.py that reflects what should happen with None, and we should try to add a test that things work with several different types.
</p>
TicketmalbMon, 14 Nov 2011 17:09:12 GMT
https://trac.sagemath.org/ticket/12020#comment:11
https://trac.sagemath.org/ticket/12020#comment:11
<p>
I checked the <code>__init__</code> methods of all pyx files in sage/matrix/ and found one which doesn't treat <code>entries==None</code> properly: <code>matrix_modn_sparse.pyx</code>, so this one should get fixed. Note that all others explicitly support <code>entries==None</code> and many do nothing when it is passed, i.e., it's the explicit fast path.
</p>
TicketwasMon, 14 Nov 2011 17:17:47 GMT
https://trac.sagemath.org/ticket/12020#comment:12
https://trac.sagemath.org/ticket/12020#comment:12
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12020#comment:11" title="Comment 11">malb</a>:
</p>
<blockquote class="citation">
<p>
I checked the <code>__init__</code> methods of all pyx files in sage/matrix/ and found one which doesn't treat <code>entries==None</code> properly: <code>matrix_modn_sparse.pyx</code>, so this one should get fixed. Note that all others explicitly support <code>entries==None</code> and many do nothing when it is passed, i.e., it's the explicit fast path.
</p>
</blockquote>
<p>
Cool. Great work checking that. I'm really happy with this now then.
</p>
TicketwasMon, 14 Nov 2011 17:20:59 GMTstatus changed
https://trac.sagemath.org/ticket/12020#comment:13
https://trac.sagemath.org/ticket/12020#comment:13
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
I give malb's trac_12020_alt.patch a positive review. Note that the patch at 12000 by Volker is isomorphic to malb's, except it also has sphinxification of the relevant docstrings (though the rest of the file is pre-sphinx) and a different doctest illustrating the same issue.
</p>
TicketjdemeyerMon, 14 Nov 2011 17:25:32 GMTstatus changed
https://trac.sagemath.org/ticket/12020#comment:14
https://trac.sagemath.org/ticket/12020#comment:14
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12020#comment:13" title="Comment 13">was</a>:
</p>
<blockquote class="citation">
<p>
I give malb's trac_12020_alt.patch a positive review.
</p>
</blockquote>
<p>
He just said that <code>matrix_modn_sparse.pyx</code> is also broken (and not yet fixed by the patch here)...
</p>
TicketwasMon, 14 Nov 2011 17:38:09 GMT
https://trac.sagemath.org/ticket/12020#comment:15
https://trac.sagemath.org/ticket/12020#comment:15
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12020#comment:14" title="Comment 14">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/12020#comment:13" title="Comment 13">was</a>:
</p>
<blockquote class="citation">
<p>
I give malb's trac_12020_alt.patch a positive review.
</p>
</blockquote>
<p>
He just said that <code>matrix_modn_sparse.pyx</code> is also broken (and not yet fixed by the patch here)...
</p>
</blockquote>
<p>
Woops -- I agree with not giving this a positive review.
</p>
<p>
It would be good to have a test that runs through a large number of base rings.
</p>
TicketmalbMon, 14 Nov 2011 18:41:30 GMTattachment set
https://trac.sagemath.org/ticket/12020
https://trac.sagemath.org/ticket/12020
<ul>
<li><strong>attachment</strong>
set to <em>trac_12020_alt.patch</em>
</li>
</ul>
TicketmalbMon, 14 Nov 2011 18:43:40 GMTstatus changed
https://trac.sagemath.org/ticket/12020#comment:16
https://trac.sagemath.org/ticket/12020#comment:16
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I added a comment to docs.py and added a return <code>if entries is None</code>. However, since e.g., <code>GF(7)(None) == 0</code>} it does not actually fix a bug, just exits slightly faster.
</p>
TicketwasMon, 14 Nov 2011 18:51:19 GMTstatus changed
https://trac.sagemath.org/ticket/12020#comment:17
https://trac.sagemath.org/ticket/12020#comment:17
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Thanks malb. It looks great to me. (And if you want to do something else for Sage-5.0, you could doctest "matrix/benchmark.py: 0% (0 of 29)" from <a class="closed ticket" href="https://trac.sagemath.org/ticket/12024" title="enhancement: 90% doctest coverage thrust metaticket (closed: fixed)">#12024</a>.)
</p>
TicketjdemeyerMon, 14 Nov 2011 21:03:39 GMTstatus, description, author changed; reviewer set
https://trac.sagemath.org/ticket/12020#comment:18
https://trac.sagemath.org/ticket/12020#comment:18
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
<li><strong>reviewer</strong>
set to <em>William Stein</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/12020?action=diff&version=18">diff</a>)
</li>
<li><strong>author</strong>
changed from <em>William Stein</em> to <em>Martin Albrecht, Jeroen Demeyer</em>
</li>
</ul>
TicketjdemeyerMon, 14 Nov 2011 21:04:13 GMTstatus, milestone changed
https://trac.sagemath.org/ticket/12020#comment:19
https://trac.sagemath.org/ticket/12020#comment:19
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-duplicate/invalid/wontfix</em> to <em>sage-4.8</em>
</li>
</ul>
<p>
Reviewer patch adding extra doctests like William suggested, needs_review.
</p>
TicketwasMon, 14 Nov 2011 21:07:33 GMT
https://trac.sagemath.org/ticket/12020#comment:20
https://trac.sagemath.org/ticket/12020#comment:20
<p>
It would be nice to define a list of 100 (say) various rings in a file in the tests directory. Then the test could be
</p>
<pre class="wiki">sage: from sage.tests.examples import commutative_rings0
sage: for R in commutative_rings0:
do something
</pre><p>
Then people could add new rings to that list, and tests all over may benefit.
</p>
<p>
And in addition to rings, we could assemble many other lists of examples of objects in that file.
</p>
<p>
This could be on another ticket.
</p>
TicketjdemeyerTue, 15 Nov 2011 09:06:54 GMTattachment set
https://trac.sagemath.org/ticket/12020
https://trac.sagemath.org/ticket/12020
<ul>
<li><strong>attachment</strong>
set to <em>12020_review.patch</em>
</li>
</ul>
TicketjdemeyerTue, 15 Nov 2011 09:08:12 GMTauthor changed
https://trac.sagemath.org/ticket/12020#comment:21
https://trac.sagemath.org/ticket/12020#comment:21
<ul>
<li><strong>author</strong>
changed from <em>Martin Albrecht, Jeroen Demeyer</em> to <em>Martin Albrecht, Volker Braun, Jeroen Demeyer</em>
</li>
</ul>
<p>
I added Volker's patch from <a class="closed ticket" href="https://trac.sagemath.org/ticket/12000" title="defect: block_matrix over cyclotomics fails (closed: duplicate)">#12000</a> here.
</p>
TicketjdemeyerSat, 19 Nov 2011 12:00:40 GMTcc set
https://trac.sagemath.org/ticket/12020#comment:22
https://trac.sagemath.org/ticket/12020#comment:22
<ul>
<li><strong>cc</strong>
<em>vbraun</em> added
</li>
</ul>
<p>
*ping* needs review
</p>
TicketvbraunSat, 19 Nov 2011 12:35:01 GMTstatus, reviewer changed
https://trac.sagemath.org/ticket/12020#comment:23
https://trac.sagemath.org/ticket/12020#comment:23
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
changed from <em>William Stein</em> to <em>William Stein, Jeroen Demeyer, Volker Braun,</em>
</li>
</ul>
<p>
Looks good!
</p>
TicketjdemeyerMon, 21 Nov 2011 10:45:42 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/12020#comment:24
https://trac.sagemath.org/ticket/12020#comment:24
<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-4.8.alpha3</em>
</li>
</ul>
Ticket