#5241 closed defect (fixed)
[with patch, positive review] Matrix Group sometimes assumes base ring is a field
Reported by: | kcrisman | Owned by: | joyner |
---|---|---|---|
Priority: | major | Milestone: | sage-4.0.1 |
Component: | group theory | Keywords: | |
Cc: | joyner | Merged in: | 4.0.1.alpha0 |
Authors: | David Joyner | Reviewers: | Alex Ghitza |
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
From sage-support:
sage: M1 = matrix(ZZ,2,[[-1,0],[0,1]]) sage: M2 = matrix(ZZ,2,[[1,0],[0,-1]]) sage: M3 = matrix(ZZ,2,[[-1,0],[0,-1]]) sage: MG = MatrixGroup([M1, M2, M3]) sage: MG.order() 4 sage: MG.list() Traceback (click to the left for traceback) ... AttributeError: 'sage.rings.integer_ring.IntegerRing_class' object has no attribute 'prime_subfield'
The offending code is in groups/matrix_gps/matrix_group.py where the problem is in the list method of MatrixGroup?.
429 F = self.field_of_definition() 430 n = F.degree() 431 p = F.characteristic() 432 a = F.prime_subfield().multiplicative_generator() 433 b = F.multiplicative_generator()
In the class definition of MatrixGroup?, any base ring is allowed. But at least this particular method (others?) assume that the base ring is in fact in a field.
Since a and b above are definitely used later in list(), as this all calls GAP, someone with a good knowledge of GAP should address this - and look for other places it's assumed that the base ring is a field and at least catch that exception with a better error message.
Attachments (2)
Change History (9)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Summary changed from Matrix Group sometimes assumes base ring is a field to [with patch, needs review] Matrix Group sometimes assumes base ring is a field
This passes sage -t but sage -testall has lots of (unrelated I think) failures. It simply fixes the bug reported and adds the required docstring examples.
comment:2 Changed 11 years ago by
Will this need rebase against 3.4?
comment:3 Changed 11 years ago by
- Cc joyner added
- Summary changed from [with patch, needs review] Matrix Group sometimes assumes base ring is a field to [with patch, needs work] Matrix Group sometimes assumes base ring is a field
Against 3.4:
patching file sage/groups/matrix_gps/matrix_group.py Hunk #1 FAILED at 410 1 out of 3 hunks FAILED -- saving rejects to file sage/groups/matrix_gps/matrix_group.py.rej abort: patch failed to apply
The hunk that didn't apply was the documentation hunk, as it turns out, unsurprisingly.
So it does need a rebase. Sorry it took a while to be able to check this; it's sometimes complicated for me to do these things in a timely fashion, particularly with the 3.4 upgrade this was the case.
comment:4 Changed 11 years ago by
- Summary changed from [with patch, needs work] Matrix Group sometimes assumes base ring is a field to [with patch, needs rebase] Matrix Group sometimes assumes base ring is a field
comment:5 Changed 11 years ago by
- Summary changed from [with patch, needs rebase] Matrix Group sometimes assumes base ring is a field to [with patch, positive review] Matrix Group sometimes assumes base ring is a field
I have rebased the patch against 4.0 and tested it. Looks good to me.
Apply only trac_5241-rebased.patch
.
comment:6 Changed 11 years ago by
- Resolution set to fixed
- Status changed from new to closed
Merged in 4.0.1.alpha0.
comment:7 Changed 11 years ago by
- Merged in set to 4.0.1.alpha0
- Reviewers set to Alex Ghitza
to be applied to sage-3.3.alpha6