MakeMagmaWithInversesByFiniteGenerators shouldn't set IsFinitelyGeneratedGroup

[fingolfin]

Consider this example:

gap> A:=[[1,2,3,4],[2,1,4,2],[3,4,1,3],[4,2,3,1]];
[ [ 1, 2, 3, 4 ], [ 2, 1, 4, 2 ], [ 3, 4, 1, 3 ], [ 4, 2, 3, 1 ] ]
gap> M:=MagmaWithInversesByMultiplicationTable(A);
<magma-with-inverses with 4 generators>
gap> IsFinitelyGeneratedGroup(M);
true
gap> IsGroup(M);
false

It is quite obvious that M is no group from the multiplication table (the 2nd and 3rd row are not permutations). Yet the magma "thinks" that it is in the filter IsFinitelyGeneratedGroup, which seems like a contradiction.

The problem is in MakeMagmaWithInversesByFiniteGenerators, which sets this filter unconditionally. This can be traced back to this commits (from the our "secret" internal repository):

commit ac50226cb8a39c978d43243a5546822fc2a2f2af
Author: Alexander Hulpke <[email protected]>
Date:   1999-04-29 09:41:11 +0000

    Changed the installation of `MagmaByGenerators' methods to the case of a
    finite generators list. This permits to set the `IsFinitelyGenerated' filter
    and thus gets rid of immediate method calls.

diff --git a/lib/magma.gi b/lib/magma.gi
index c5f731d3a..70550e70b 100644
--- a/lib/magma.gi
+++ b/lib/magma.gi
@@ -15,7 +15,6 @@ Revision.magma_gi :=
 
 #############################################################################
 ##
-
 #M  PrintObj( <M> ) . . . . . . . . . . . . . . . . . . . . . . print a magma
 ##
 InstallMethod( PrintObj,
@@ -606,14 +605,16 @@ InstallOtherMethod( MagmaWithOneByGenerators,
 ##
 #M  MagmaWithInversesByGenerators( <gens> ) . . . . . . . .  for a collection
 ##
-MakeMagmaWithInversesByGenerators:=function(family,gens)
+MakeMagmaWithInversesByFiniteGenerators:=function(family,gens)
 local M;
   if not IsBound(family!.defaultMagmaWithInversesByGeneratorsType) then
     family!.defaultMagmaWithInversesByGeneratorsType :=
       NewType( FamilyObj( gens ),
 		IsMagmaWithInverses and IsAttributeStoringRep 
-		and HasGeneratorsOfMagmaWithInverses);
+		and HasGeneratorsOfMagmaWithInverses 
+		and IsFinitelyGeneratedGroup);
   fi;
+
   M:=rec();
   ObjectifyWithAttributes( M,family!.defaultMagmaWithInversesByGeneratorsType,
     GeneratorsOfMagmaWithInverses, AsList( gens ) );
@@ -621,9 +622,9 @@ local M;
 end;
 
 InstallMethod( MagmaWithInversesByGenerators, "for collection", true,
-    [ IsCollection ] , 0,
+    [ IsCollection and IsFinite] , 0,
 function( gens )
-  return MakeMagmaWithInversesByGenerators(FamilyObj(gens),gens);
+  return MakeMagmaWithInversesByFiniteGenerators(FamilyObj(gens),gens);
 end);
 
 #############################################################################
@@ -631,13 +632,13 @@ end);
 #M  MagmaWithInversesByGenerators( <Fam>, <gens> )  . . . for family and list
 ##
 InstallOtherMethod( MagmaWithInversesByGenerators, "for family and list",
-    true, [ IsFamily, IsList ], 0,
+    true, [ IsFamily, IsList and IsFinite], 0,
 function( family, gens )
 local M;
   if not ( IsEmpty(gens) or IsIdenticalObj( FamilyObj(gens), family ) ) then
     Error( "<family> and family of <gens> do not match" );
   fi;
-  return MakeMagmaWithInversesByGenerators(family,gens);
+  return MakeMagmaWithInversesByFiniteGenerators(family,gens);
 end );
 
 

[ThomasBreuer]

@hulpke:
What about changing the declaration of IsFinitelyGeneratedGroup as follows.

Declare a property IsFinitelyGeneratedMagmaWithInverses,
with an implication to IsMagmaWithInverses,
and define IsFinitelyGeneratedGroup as IsFinitelyGeneratedMagmaWithInverses and IsGroup.

Then the type in MakeMagmaWithInversesByFiniteGenerators can be changed to haveIsFinitelyGeneratedMagmaWithInverses set instead of IsFinitelyGeneratedGroup,
and the problem is gone.

[fingolfin]

@ThomasBreuer's suggestion sounds sensible to me. I only wonder if we should perhaps use IsFinitelyGeneratedMagma instead of IsFinitelyGeneratedMagmaWithInverses, as this is more universal -- a magma-with-inverses that is finitely generated is also finitely generated as a magma, after all.

[fingolfin]

Going even further, since we also defined IsCommutative and IsAssociative as the basic properties "for magmas", one could even call the new property IsFinitelyGenerated. Not saying we should, just pointing out that it'd fit with these existing properties.

[ThomasBreuer]

@fingolfin:
The only point is to separate the property IsAssociative from the structural categories IsMagma, IsMagmaWithInverses etc.
Defining IsFinitelyGeneratedMagma is indeed a better idea than defining IsFinitelyGeneratedMagmaWithInverses.

I would not be in favour of IsFinitelyGenerated, this might lead to confusion in the situation of rings and ideals where it makes a difference whether a domain is finitely generated as an additive structure, as a multiplicative structure, as a ring, as an ideal, etc.

[fingolfin]

Unfortunately, there are some packages which do this:

    SetIsFinitelyGeneratedGroup( G, false );

This does not work if IsFinitelyGeneratedGroup is a synonym for an and-filter like IsFinitelyGeneratedMagma and IsGroup, at least not with the default setter. One workaround would be a custom setter, which checks whether IsGroups is set (otherwise, produce an error), and then calls SetIsFinitelyGenerated(G, value).

Or, we do it less complicated, and instead look into dropping the IsFinitelyGeneratedGroup from MakeMagmaWithInversesByFiniteGenerators and see what happens? Note that we already have the following immediate method in lib/grp.gi, which should ensure that groups automatically get the IsFinitelyGeneratedGroup property if they are created via MakeMagmaWithInversesByFiniteGenerators (also note that this method is actually wrong):

InstallImmediateMethod( IsFinitelyGeneratedGroup,
    IsGroup and HasGeneratorsOfGroup, 0,
    G -> IsFinite( GeneratorsOfGroup( G ) ) );

[ThomasBreuer]

@fingolfin:
Thanks for inspecting this.

The mentioned use of SetIsFinitelyGeneratedGroup shows once more that it was a bad idea to introduce the property IsFinitelyGeneratedGroup.
(Do we have a chance to get out from this situation?)

In order to fix MakeMagmaWithInversesByFiniteGenerators,
I would then propose to check in this function whether the object created by ObjectifyWithAttributes
has IsAssociative set, and in this case to set IsFinitelyGeneratedGroup.

Of course the immediate method for IsFinitelyGeneratedGroup should be fixed.
(It is in fact the default method for this operation,
note that immediate methods are also installed as ordinary methods.)

[fingolfin]

@ThomasBreuer please take a look at PRs #2275 and #2276; one fixes the immediate method for IsFinitelyGeneratedGroup; the other modifies MakeMagmaWithInversesByFiniteGenerators in a way similar to what you suggest, but it checks the family, instead of the constructed object (if we called ObjectifyWithAttributes first, then the immediate method for IsFinitelyGeneratedGroup would already have been triggered, but avoiding that seems to be the whole (only?) point of setting IsFinitelyGeneratedGroup explicitly, as far as I can tell.

Personally, I would be content to just let the immediate method for IsFinitelyGeneratedGroup trigger, and get rid of the tricks in MakeMagmaWithInversesByFiniteGenerators, but since I don't know why it was felt necessary to avoid this in the first place, I'll not touch it -- perhaps @hulpke can illuminate this a bit?

[james-d-mitchell]

I looked at PR #2276, and I think it'd be better to get rid of the tricks in MakeMagmaWithInverseByFiniteGenerators too unless there is some really good reason for doing this.