Opened 8 years ago

Last modified 8 years ago

#12020 closed defect

bug in zero_matrix rewrite in matrix_space.py — at Version 18

Reported by: was Owned by: jason, was
Priority: blocker Milestone: sage-4.8
Component: linear algebra Keywords:
Cc: vbraun Merged in:
Authors: Martin Albrecht, Jeroen Demeyer Reviewers: William Stein
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Observe this:

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'>

Debugging we see that the problem is that this code in matrix_space.py:

    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)

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...

This bug was introduced in #11589.

Apply trac_12020_alt.patch and 12020_review.patch

Change History (21)

comment:1 Changed 8 years ago by was

I figured this out. This bug was introduced by #11589 (by Simon King and Martin Albrecht). Basically the zero_matrix() method's code was "mangled/changed" (?) when being copied into the init 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 int(None) is an error. Also, this is a weird change since the zero_matrix method in the new code still uses 0.

Patch coming up.

comment:2 Changed 8 years ago by was

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 #11589, which is probably why this bug slipped through our test framework (we *really* need a random larger testing framework!):

sage: A = MatrixSpace(CyclotomicField(4),60,30)(0)
sage: A.augment(A)
boom!

comment:3 Changed 8 years ago by was

  • Summary changed from major (BLOCKER!) bug probably in matrix_space.py to bug in zero_matrix rewrite in matrix_space.py

Changed 8 years ago by was

Changed 8 years ago by jdemeyer

Same patch with corrected doc formatting

comment:4 Changed 8 years ago by jdemeyer

  • Authors set to William Stein
  • Description modified (diff)
  • Status changed from new to needs_review

comment:5 follow-up: Changed 8 years ago by malb

I have attached an alternative solution: instead of switching back to 0 instead of None make Matrix_cyclo_dense deal with None properly. The reason we switched to None was because this is faster since most (?, perhaps only some) matrix classes do nothing if None 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 0.

comment:6 in reply to: ↑ 5 Changed 8 years ago by jdemeyer

Replying to malb:

Of course, one could also fix those matrix classes and switch back to 0.

I slightly prefer that because it seems safer to me.

comment:7 follow-up: Changed 8 years ago by vbraun

I found and fixed this bug earlier in #12000. Of course nobody ever reviews my patches, sniff.

comment:8 in reply to: ↑ 7 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.8 to sage-duplicate/invalid/wontfix

Replying to vbraun:

I found and fixed this bug earlier in #12000. Of course nobody ever reviews my patches, sniff.

Especially with such a nice ticket number...

Proposing to close this as "duplicate"

comment:9 Changed 8 years ago by malb

If "this" is "this ticket" then I agree.

comment:10 Changed 8 years ago by was

The fix for #12000 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 #12000 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.

comment:11 follow-up: Changed 8 years ago by malb

I checked the __init__ methods of all pyx files in sage/matrix/ and found one which doesn't treat entries==None properly: matrix_modn_sparse.pyx, so this one should get fixed. Note that all others explicitly support entries==None and many do nothing when it is passed, i.e., it's the explicit fast path.

comment:12 in reply to: ↑ 11 Changed 8 years ago by was

Replying to malb:

I checked the __init__ methods of all pyx files in sage/matrix/ and found one which doesn't treat entries==None properly: matrix_modn_sparse.pyx, so this one should get fixed. Note that all others explicitly support entries==None and many do nothing when it is passed, i.e., it's the explicit fast path.

Cool. Great work checking that. I'm really happy with this now then.

comment:13 follow-up: Changed 8 years ago by was

  • Status changed from needs_review to positive_review

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.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Replying to was:

I give malb's trac_12020_alt.patch a positive review.

He just said that matrix_modn_sparse.pyx is also broken (and not yet fixed by the patch here)...

comment:15 in reply to: ↑ 14 Changed 8 years ago by was

Replying to jdemeyer:

Replying to was:

I give malb's trac_12020_alt.patch a positive review.

He just said that matrix_modn_sparse.pyx is also broken (and not yet fixed by the patch here)...

Woops -- I agree with not giving this a positive review.

It would be good to have a test that runs through a large number of base rings.

Changed 8 years ago by malb

comment:16 Changed 8 years ago by malb

  • Status changed from needs_work to needs_review

I added a comment to docs.py and added a return if entries is None. However, since e.g., GF(7)(None) == 0} it does not actually fix a bug, just exits slightly faster.

comment:17 Changed 8 years ago by was

  • Status changed from needs_review to positive_review

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 #12024.)

comment:18 Changed 8 years ago by jdemeyer

  • Authors changed from William Stein to Martin Albrecht, Jeroen Demeyer
  • Description modified (diff)
  • Reviewers set to William Stein
  • Status changed from positive_review to needs_work
Note: See TracTickets for help on using tickets.