[with patch, positive review] Matrix Group sometimes assumes base ring is a field

[kcrisman]

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.

[wdj]

to be applied to sage-3.3.alpha6

[wdj]

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.

[kcrisman]

Will this need rebase against 3.4?

[kcrisman]

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.

[AlexGhitza]

I have rebased the patch against 4.0 and tested it. Looks good to me.

Apply only trac_5241-rebased.patch.

[AlexGhitza]

rebased against 4.0

[mhansen]

Merged in 4.0.1.alpha0.