Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

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

trac_5241-matrix-gp-list.patch (2.0 KB) - added by wdj 11 years ago.
to be applied to sage-3.3.alpha6
trac_5241-rebased.patch (2.2 KB) - added by AlexGhitza 11 years ago.
rebased against 4.0

Download all attachments as: .zip

Change History (9)

Changed 11 years ago by wdj

to be applied to sage-3.3.alpha6

comment:1 Changed 11 years ago by wdj

  • 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 kcrisman

Will this need rebase against 3.4?

comment:3 Changed 11 years ago by kcrisman

  • 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 mabshoff

  • 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 AlexGhitza

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

Changed 11 years ago by AlexGhitza

rebased against 4.0

comment:6 Changed 11 years ago by mhansen

  • Resolution set to fixed
  • Status changed from new to closed

Merged in 4.0.1.alpha0.

comment:7 Changed 11 years ago by mhansen

  • Authors set to David Joyner
  • Merged in set to 4.0.1.alpha0
  • Reviewers set to Alex Ghitza
Note: See TracTickets for help on using tickets.