Add GroupMixinLibGAP as a base class for finitely presented groups

[sagetrac-dshurbert]

Adding GroupMixinLibGAP to the class FinitelyPresentedGroup adds basic functionality
to finitely presented groups such as is_abelian, which previously threw a NotImplementedError.

sage: F = FreeGroup(2)
sage: G = F / [F([1,2,-1,-2])]
sage: G.is_abelian()
True

Apply

1 attachment: trac_14913_fpgroups_GroupMixinLibGAP.patch

Depends on #14519

Upstream: Reported upstream. Developers acknowledge bug.

CC: @rbeezer @vbraun @miguelmarco

Component: group theory

Keywords: group presentations

Author: Davis Shurbert

Reviewer: Volker Braun

Merged: sage-5.12.beta5

Issue created by migration from https://trac.sagemath.org/ticket/14913

[sagetrac-dshurbert]

Attachment: trac_14913_is_abelian.patch.gz

Patch

[vbraun]

comment:2

This is already implemented in GroupMixinLibGAP, you probably just have to include it in the base classes for finitely presented groups (the mixin was added later to Sage).

[sagetrac-dshurbert]

comment:3

I see, my fault for not having a new enough release. I'm still waiting on my machine to finish building 5.11.beta3, as I am still on 5.10 at home. I will make the necessary changes as soon as possible.

[sagetrac-dshurbert]

comment:5

Just changed the patch from wrapping GAP's IsAbelian. GroupMixinLibGAP has been added to the class declaration for FinitelyPresentedGroups and works as intended.

[vbraun]

Reviewer: Volker Braun

[vbraun]

comment:7

Looks good to me.

[sagetrac-dshurbert]

comment:8

I discovered the following behavior while writing doctests for a direct_product function for finitely presented groups.

sage: from sage.groups.finitely_presented import wrap_FpGroup
sage: F = FreeGroup("a")
sage: G = FreeGroup("b")
sage: C4 = F / [F.0^4]
sage: C3 = G / [G.0^3]

sage: C12 = wrap_FpGroup(libgap.DirectProduct([C3.gap(), C4.gap()]))
sage: C12
Finitely presented group < f1, f2 | f1^3, f2^4, f1^-1*f2^-1*f1*f2 >
sage: C12.is_isomorphic(F / [F.0^12])
False
sage: C12.as_permutation_group().is_isomorphic(CyclicPermutationGroup(12))
True

# Again, constructing C12 by hand as C3 x C4.
sage: K = FreeGroup(2)
sage: C_12 = K / [K.0^4, K.1^3, K([-1,-2,1,2])]
sage: C_12
Finitely presented group < x0, x1 | x0^4, x1^3, x0^-1*x1^-1*x0*x1 >
sage: C_12.is_isomorphic(F / [F.0^12])
False
sage: C_12.as_permutation_group().is_isomorphic(CyclicPermutationGroup(12))
True

To replicate this behavior you must apply the patch attached to this ticket, #14913.

This failure of the is_isomorphic call seems to recur in similar instances of cyclic groups being constructed as a direct product. I'm not sure if I should make a new ticket for this apparent bug, but at the very least it seems as though it should halt this ticket.

[vbraun]

comment:10

You also need to add the check=<bool> argument to the finitely presented group ctor:

sage: list(C12)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
...
/home/vbraun/opt/sage-5.11.beta3/local/lib/python2.7/site-packages/sage/groups/finitely_presented.pyc in _element_constructor_(self, *args, **kwds)
    697         if P is self._free_group:
    698             return self.element_class(x.Tietze(), parent=self, **kwds)
--> 699         return self.element_class(x, parent=self, **kwds)
    700 
    701     @cached_method

TypeError: __init__() got an unexpected keyword argument 'check'

[vbraun]

comment:11

The failure to find the isomorphism between the finitely presented groups looks like a GAP bug. I've reported it at http://tracker.gap-system.org/issues/424

[vbraun]

Upstream: Reported upstream. No feedback yet.

[sagetrac-dshurbert]

comment:12

Added check=True as last argument for FinitelyPresentedGroupElement python constructor. Adding this argument to __init__ for FinitelyPresentedGroup did not appear to have the desired affect. The examples below now run smoothly.

sage: G = FreeGroup("a")
sage: F = G / [G([1,1,1,1,1])]
sage: F.is_abelian()
True
sage: F.order()
5
sage: [i for i in F]
[1, a, a^-1, a^2, a^-2]
sage: F([1,1,1]).inverse()         
a^-3
sage: F([1,1,1]).inverse() in F
True

Is this what you meant by adding the check=<bool> argument? If so then ready for review.

[vbraun]

comment:14

Yes, thats what I meant: the element constructor needs to accept a dummy check argument.

Upstream has targeted the bug for the next minor release.

[vbraun]

Changed upstream from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

[vbraun]

comment:15

Patchbot:
apply trac_14913_fpgroups_GroupMixinLibGAP.patch​

[vbraun]

comment:16

Looks good to me

[jdemeyer]

Dependencies: #14519

[jdemeyer]

comment:18

This needs to be rebased to #14519.

[sagetrac-dshurbert]

comment:19

Rebased to #14519, ready for review.

[sagetrac-dshurbert]

Replacement patch, rebased to #14519

[tscrim]

comment:20

Attachment: trac_14913_fpgroups_GroupMixinLibGAP.patch.gz

Applies cleanly for me and all tests pass, so I'm setting this back to positive review.

[jdemeyer]

Merged: sage-5.12.beta5