Opened 8 years ago

Closed 8 years ago

#12020 closed defect (fixed)

bug in zero_matrix rewrite in matrix_space.py

Reported by: was Owned by: jason, was
Priority: blocker Milestone: sage-4.8
Component: linear algebra Keywords:
Cc: vbraun Merged in: sage-4.8.alpha3
Authors: Martin Albrecht, Volker Braun, Jeroen Demeyer Reviewers: William Stein, Jeroen Demeyer, Volker Braun,
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

Attachments (4)

trac_12020.patch (1.2 KB) - added by was 8 years ago.
trac_12020.2.patch (1.2 KB) - added by jdemeyer 8 years ago.
Same patch with corrected doc formatting
trac_12020_alt.patch (2.2 KB) - added by malb 8 years ago.
12020_review.patch (3.3 KB) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (28)

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

comment:19 Changed 8 years ago by jdemeyer

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

Reviewer patch adding extra doctests like William suggested, needs_review.

comment:20 Changed 8 years ago by was

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

sage: from sage.tests.examples import commutative_rings0
sage: for R in commutative_rings0: 
         do something

Then people could add new rings to that list, and tests all over may benefit.

And in addition to rings, we could assemble many other lists of examples of objects in that file.

This could be on another ticket.

Changed 8 years ago by jdemeyer

comment:21 Changed 8 years ago by jdemeyer

  • Authors changed from Martin Albrecht, Jeroen Demeyer to Martin Albrecht, Volker Braun, Jeroen Demeyer

I added Volker's patch from #12000 here.

comment:22 Changed 8 years ago by jdemeyer

  • Cc vbraun added

*ping* needs review

comment:23 Changed 8 years ago by vbraun

  • Reviewers changed from William Stein to William Stein, Jeroen Demeyer, Volker Braun,
  • Status changed from needs_review to positive_review

Looks good!

comment:24 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.8.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.