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 )
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)
Change History (28)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
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
- 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
comment:4 Changed 8 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:5 follow-up: ↓ 6 Changed 8 years ago by
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
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: ↓ 8 Changed 8 years ago by
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
- Milestone changed from sage-4.8 to sage-duplicate/invalid/wontfix
comment:9 Changed 8 years ago by
If "this" is "this ticket" then I agree.
comment:10 Changed 8 years ago by
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: ↓ 12 Changed 8 years ago by
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
Replying to malb:
I checked the
__init__
methods of all pyx files in sage/matrix/ and found one which doesn't treatentries==None
properly:matrix_modn_sparse.pyx
, so this one should get fixed. Note that all others explicitly supportentries==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: ↓ 14 Changed 8 years ago by
- 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: ↓ 15 Changed 8 years ago by
- 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
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
comment:16 Changed 8 years ago by
- 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
- 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
- Description modified (diff)
- Reviewers set to William Stein
- Status changed from positive_review to needs_work
comment:19 Changed 8 years ago by
- 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
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
comment:23 Changed 8 years ago by
- 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
- Merged in set to sage-4.8.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
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.