Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce impact of immediate methods to calculations with groups #2387

Closed
wants to merge 1 commit into from
Closed

Reduce impact of immediate methods to calculations with groups #2387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/coll.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1371,6 +1371,10 @@ InstallFactorMaintenance( IsTrivial,
##
DeclareProperty( "IsNonTrivial", IsCollection );

# true methods to avoid immediate methods
InstallTrueMethod(HasIsTrivial,IsNonTrivial);
InstallTrueMethod(HasIsNonTrivial,IsTrivial);


#############################################################################
##
Expand Down
48 changes: 41 additions & 7 deletions lib/coll.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ InstallMethod( PrintObj,
##
InstallImmediateMethod( IsEmpty,
IsCollection and HasSize, 0,
C -> Size( C ) = 0 );
C->Size( C ) = 0);

InstallMethod( IsEmpty,
"for a collection",
Expand All @@ -120,17 +120,17 @@ InstallMethod( IsTrivial,
[ IsCollection ],
C -> Size( C ) = 1 );

InstallImmediateMethod( IsTrivial,
IsCollection and HasIsNonTrivial, 0,
InstallMethod( IsTrivial,
[IsCollection and HasIsNonTrivial], 0,
C -> not IsNonTrivial( C ) );


#############################################################################
##
#M IsNonTrivial( <C> ) . . . . . . . . . test if a collection is nontrivial
##
InstallImmediateMethod( IsNonTrivial,
IsCollection and HasIsTrivial, 0,
InstallMethod( IsNonTrivial,
[IsCollection and HasIsTrivial], 0,
C -> not IsTrivial( C ) );

InstallMethod( IsNonTrivial,
Expand Down Expand Up @@ -169,8 +169,12 @@ InstallMethod( IsWholeFamily,
##
#M Size( <C> ) . . . . . . . . . . . . . . . . . . . . size of a collection
##
InstallImmediateMethod( Size,
IsCollection and HasIsFinite and IsAttributeStoringRep, 0,
# This used to be an immediate method. It was replaced by an ordinary
# method, as the immediate method would get called for every group that
# knows it is finite but does not know its size -- e.g. permutation, pc.
# The benefit of this is minimal beyond showing off a feature.
InstallMethod( Size,true, [IsCollection and HasIsFinite],
100, # rank above object-specific methods
function ( C )
if IsFinite( C ) then
TryNextMethod();
Expand Down Expand Up @@ -3049,6 +3053,36 @@ end);
InstallMethod( CanComputeIsSubset,"default: no, unless identical",
[IsObject,IsObject],IsIdenticalObj);

# This setter method is installed to implement filter settings in response
# to an objects size as part of setting the size. This used to be handled
# instead by immediate methods, but in a situation as here it would trigger
# multiple immediate methods, several of which could apply and each changing
# the type of the object. Doing so can be costly and thus should be
# avoided.
InstallOtherMethod(SetSize,true,[IsObject and IsAttributeStoringRep,IsObject],
100, # override system setter
function(obj,sz)
local filt;
if HasSize(obj) and Size(obj)<>sz then
if AssertionLevel()>2 then
# Make this an ordinary error (not ErrorNoReturn as suggested) to
# preserve all debugging options -- even use `return` to investigate
# what would have happened before this methods was introduced.
Error("size of ",obj," already set to ",Size(obj),
", cannot be changed to ",sz);
fi;
return;
fi;
if sz=0 then filt:=IsEmpty and IsNonTrivial; # IsNonTrivial hold
elif sz=1 then filt:=IsTrivial;
elif sz=infinity then filt:=IsNonTrivial and HasIsFinite;
else filt:=IsNonTrivial and IsFinite;
fi;
filt:=filt and HasSize;
obj!.Size:=sz;
SetFilterObj(obj,filt);
end);

#############################################################################
##
#E
35 changes: 19 additions & 16 deletions lib/csetgrp.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,18 +411,17 @@ end);
InstallOtherMethod(DoubleCoset,"with size",true,
[IsGroup,IsObject,IsGroup,IsPosInt],0,
function(U,g,V,sz)
local d,fam;
local d,fam,typ;
fam:=FamilyObj(U);
if not IsBound(fam!.doubleCosetsDefaultSizeType) then
fam!.doubleCosetsDefaultSizeType:=NewType(fam,IsDoubleCosetDefaultRep
and HasSize and HasIsFinite and IsFinite
typ:=NewType(fam,IsDoubleCosetDefaultRep
and HasIsFinite and IsFinite
and HasLeftActingGroup and HasRightActingGroup
and HasRepresentative);
fi;
d:=rec();
ObjectifyWithAttributes(d,fam!.doubleCosetsDefaultSizeType,
LeftActingGroup,U,RightActingGroup,V,Representative,g,
Size,sz);
ObjectifyWithAttributes(d,typ,
LeftActingGroup,U,RightActingGroup,V,Representative,g);
SetSize(d,sz); # Size has private setter which will cause problems with
# HasSize triggering an immediate method.
return d;
end);

Expand Down Expand Up @@ -574,21 +573,25 @@ end);
InstallMethod(RightCoset,"use subgroup size",IsCollsElms,
[IsGroup and HasSize,IsObject],0,
function(U,g)
local d,fam;
local d,fam,typ;
# noch tests...

fam:=FamilyObj(U);
if not IsBound(fam!.rightCosetsDefaultSizeType) then
fam!.rightCosetsDefaultSizeType:=NewType(fam,IsRightCosetDefaultRep and
HasActingDomain and HasFunctionAction and HasRepresentative and
HasSize and HasCanonicalRepresentativeDeterminatorOfExternalSet);
fi;
typ:=NewType(fam,IsRightCosetDefaultRep and
HasActingDomain and HasFunctionAction and HasRepresentative
and HasCanonicalRepresentativeDeterminatorOfExternalSet);

d:=rec();
ObjectifyWithAttributes(d,fam!.rightCosetsDefaultSizeType,
ObjectifyWithAttributes(d,typ,
ActingDomain,U,FunctionAction,OnLeftInverse,Representative,g,
Size,Size(U),CanonicalRepresentativeDeterminatorOfExternalSet,
CanonicalRepresentativeDeterminatorOfExternalSet,
RightCosetCanonicalRepresentativeDeterminator);
# We cannot set the size in the previous ObjectifyWithAttributes as there is
# a custom setter method (the one added in this commit). In such a case
# ObjectifyWith Attributes just does `Objectify` and calls all setters
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"ObjectifyWith Attributes" -> "ObjectifyWithAttributes" (one word)

# separately which is what we want to avoid here.
SetSize(d,Size(U));

return d;
end);

Expand Down
2 changes: 2 additions & 0 deletions lib/ctbl.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3584,6 +3584,8 @@ DeclareGlobalVariable( "CharacterTableDisplayDefaults" );
## tbl:=rec();
## tbl.Irr:=
## [ [ 1, 1 ], [ 1, -1 ] ];
## tbl.IsFinite:=
## true;
## tbl.NrConjugacyClasses:=
## 2;
## tbl.Size:=
Expand Down
8 changes: 8 additions & 0 deletions lib/grp.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,7 @@ DeclareProperty( "IsPerfectGroup", IsGroup );
InstallFactorMaintenance( IsPerfectGroup,
IsGroup and IsPerfectGroup, IsObject, IsGroup );

InstallTrueMethod( IsPerfectGroup, IsGroup and IsTrivial );

#############################################################################
##
Expand Down Expand Up @@ -783,6 +784,8 @@ InstallFactorMaintenance( IsSolvableGroup,
InstallTrueMethod( IsSolvableGroup, IsMonomialGroup );
InstallTrueMethod( IsSolvableGroup, IsSupersolvableGroup );

InstallTrueMethod( HasIsPerfectGroup, IsGroup and IsSolvableGroup and IsNonTrivial );

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view



#############################################################################
##
Expand Down Expand Up @@ -3566,6 +3569,11 @@ DeclareOperation( "GroupWithGenerators", [ IsCollection ] );
DeclareOperation( "GroupWithGenerators",
[ IsCollection, IsMultiplicativeElementWithInverse ] );

#F MakeGroupyType( <fam>, <filt>, <gens>, <isgroup> )
# type creator function to incorporate basic deductions so immediate methods
# are not needed
DeclareGlobalFunction("MakeGroupyType");


#############################################################################
##
Expand Down
153 changes: 79 additions & 74 deletions lib/grp.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ InstallImmediateMethod( IsFinitelyGeneratedGroup,
##
#M IsCyclic( <G> ) . . . . . . . . . . . . . . . . test if a group is cyclic
##
InstallImmediateMethod( IsCyclic, IsGroup and HasGeneratorsOfGroup, 0,
# This used to be an immediate method. It was replaced by an ordinary
# method since the flag is typically set when creating the group.
InstallMethod( IsCyclic, true, [IsGroup and HasGeneratorsOfGroup], 0,
function( G )
if Length( GeneratorsOfGroup( G ) ) = 1 then
return true;
Expand All @@ -43,10 +45,14 @@ InstallMethod( IsCyclic,
"generic method for groups",
[ IsGroup ],
function ( G )
local a;

# if <G> has a generator list of length 1 then <G> is cyclic
if HasGeneratorsOfGroup( G ) and Length( GeneratorsOfGroup(G) ) = 1 then
SetMinimalGeneratingSet(G,GeneratorsOfGroup(G));
a:=GeneratorsOfGroup(G)[1];
if CanEasilyCompareElements(a) and not IsOne(a) then
SetMinimalGeneratingSet(G,GeneratorsOfGroup(G));
fi;
return true;

# if <G> is not commutative it is certainly not cyclic
Expand Down Expand Up @@ -433,11 +439,6 @@ InstallMethod( IsNilpotentGroup,
##
#M IsPerfectGroup( <G> ) . . . . . . . . . . . . test if a group is perfect
##
InstallImmediateMethod( IsPerfectGroup,
IsSolvableGroup and HasIsTrivial,
0,
IsTrivial );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this, instead of turning this into a plain method? Wouldn't that still be useful?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I don't think it's necessary to add a "this used to be an immediate method ..." comment)

InstallImmediateMethod( IsPerfectGroup,
IsGroup and HasIsAbelian and IsSimpleGroup,
0,
Expand Down Expand Up @@ -4283,6 +4284,39 @@ local s,b;
return s;
end );

#F MakeGroupyType( <fam>, <filt>, <gens>, <id>, <isgroup> )
# type creator function to incorporate basic deductions so immediate methods
# are not needed. Parameters are family, filter to start with, generator
# list, is it indeed a group (or only magma)?
InstallGlobalFunction(MakeGroupyType,
function(fam,filt,gens,id,isgroup)

if IsFinite(gens) then
if isgroup then
filt:=filt and IsFinitelyGeneratedGroup;
fi;

if Length(gens)>0 and CanEasilyCompareElements(gens) then
if id=false then
id:=One(gens[1]);
fi;
if id<>fail then # cannot do identity in magma
if ForAny(gens,x->x<>id) then
filt:=filt and HasIsTrivial and IsNonTrivial;
if isgroup and Length(gens)<=1 then # cyclic not for magmas
filt:=filt and IsCyclic;
fi;
else
filt:=filt and IsTrivial and HasIsNonTrivial;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we now have implications IsTrivial => HasIsNonTrivial and IsNonTrivial => HasIsTrivial I'd kind of prefer if this line (and similar ones throughout this PR) were changed to e.g. filt:=filt and IsTrivial -- I find that much easier to grok; indeed, several times when reading through this PR did I misread a HasIsTrivial / IsTrivial / HasIsNonTrivial / IsNonTrivial

fi;
fi;
elif isgroup and Length(gens)<=1 then # cyclic not for magmas
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could replace this elsif by fi; if and then get rid of the identical check a few lines above.

filt:=filt and IsCyclic;
fi;
fi;
return NewType(fam,filt);
end);

#############################################################################
##
#M GroupWithGenerators( <gens> ) . . . . . . . . group with given generators
Expand All @@ -4291,86 +4325,57 @@ end );
InstallMethod( GroupWithGenerators,
"generic method for collection",
[ IsCollection ],
function( gens )
local G,fam,typ;

fam:=FamilyObj(gens);
if IsFinite(gens) then
if not IsBound(fam!.defaultFinitelyGeneratedGroupType) then
fam!.defaultFinitelyGeneratedGroupType:=
NewType(fam,IsGroup and IsAttributeStoringRep
and HasGeneratorsOfMagmaWithInverses
and IsFinitelyGeneratedGroup);
fi;
typ:=fam!.defaultFinitelyGeneratedGroupType;
else
if not IsBound(fam!.defaultGroupType) then
fam!.defaultGroupType:=
NewType(fam,IsGroup and IsAttributeStoringRep
and HasGeneratorsOfMagmaWithInverses);
fi;
typ:=fam!.defaultGroupType;
fi;
function( gens )
local G,typ;

G:=rec();
ObjectifyWithAttributes(G,typ,GeneratorsOfMagmaWithInverses,AsList(gens));
typ:=MakeGroupyType(FamilyObj(gens),
IsGroup and IsAttributeStoringRep
and HasIsEmpty and HasGeneratorsOfMagmaWithInverses,
gens,false,true);

return G;
end );
G:=rec();
ObjectifyWithAttributes(G,typ,GeneratorsOfMagmaWithInverses,AsList(gens));

return G;
end );

InstallMethod( GroupWithGenerators,
"generic method for collection and identity element",
IsCollsElms, [ IsCollection, IsMultiplicativeElementWithInverse ],
function( gens, id )
local G,fam,typ;

fam:=FamilyObj(gens);
if IsFinite(gens) then
if not IsBound(fam!.defaultFinitelyGeneratedGroupWithOneType) then
fam!.defaultFinitelyGeneratedGroupWithOneType:=
NewType(fam,IsGroup and IsAttributeStoringRep
and HasGeneratorsOfMagmaWithInverses
and IsFinitelyGeneratedGroup and HasOne);
fi;
typ:=fam!.defaultFinitelyGeneratedGroupWithOneType;
else
if not IsBound(fam!.defaultGroupWithOneType) then
fam!.defaultGroupWithOneType:=
NewType(fam,IsGroup and IsAttributeStoringRep
and HasGeneratorsOfMagmaWithInverses and HasOne);
fi;
typ:=fam!.defaultGroupWithOneType;
fi;
function( gens, id )
local G,typ;

G:=rec();
ObjectifyWithAttributes(G,typ,GeneratorsOfMagmaWithInverses,AsList(gens),
One,id);
typ:=MakeGroupyType(FamilyObj(gens),
IsGroup and IsAttributeStoringRep
and HasIsEmpty and HasGeneratorsOfMagmaWithInverses and HasOne,
gens,id,true);

return G;
G:=rec();
ObjectifyWithAttributes(G,typ,GeneratorsOfMagmaWithInverses,AsList(gens),
One,id);

return G;
end );

InstallMethod( GroupWithGenerators,
"method for empty list and element",
[ IsList and IsEmpty, IsMultiplicativeElementWithInverse ],
function( empty, id )
local G,fam,typ;
InstallMethod( GroupWithGenerators,"method for empty list and element",
[ IsList and IsEmpty, IsMultiplicativeElementWithInverse ],
function( empty, id )
local G,fam,typ;

fam:= CollectionsFamily( FamilyObj( id ) );
if not IsBound( fam!.defaultFinitelyGeneratedGroupWithOneType ) then
fam!.defaultFinitelyGeneratedGroupWithOneType:=
NewType( fam, IsGroup and IsAttributeStoringRep
and HasGeneratorsOfMagmaWithInverses
and IsFinitelyGeneratedGroup and HasOne );
fi;
typ:= fam!.defaultFinitelyGeneratedGroupWithOneType;
fam:= CollectionsFamily( FamilyObj( id ) );

G:= rec();
ObjectifyWithAttributes( G, typ,
GeneratorsOfMagmaWithInverses, empty,
One, id );
typ:=IsGroup and IsAttributeStoringRep
and HasGeneratorsOfMagmaWithInverses and HasOne and IsTrivial and
HasIsEmpty and HasIsNonTrivial;
typ:=NewType(fam,typ);

return G;
end );
G:= rec();
ObjectifyWithAttributes( G, typ,
GeneratorsOfMagmaWithInverses, empty,
One, id );

return G;
end );


#############################################################################
Expand Down
Loading