Skip to content

Fix PQuotient error for large groups. #5816

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

Draft
FriedrichRober wants to merge 5 commits into
base: master
Choose a base branch
from
+51 −15
Draft

Fix PQuotient error for large groups. #5816

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2087635
Fix PQuotient error for large groups.
FriedrichRober Oct 15, 2024
ee883b6
Apply suggestions from code review
FriedrichRober Oct 15, 2024
721958b
PQuot: Adjust documentation
FriedrichRober Nov 12, 2024
99056db
PQuot: remove broken lines in new test file
FriedrichRober Nov 12, 2024
a2b9ba2
PQuot: Fix grammar in documentation
FriedrichRober Nov 12, 2024
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
13 changes: 7 additions & 6 deletions lib/pquot.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ DeclareGlobalFunction( "AbelianPQuotient" );

#############################################################################
##
#F PQuotient(<F>, <p>[, <c>][, <logord>][, <ctype>]) . . pq of an fp group
#F PQuotient(<F>, <p>[, <c>][, <logord>][, <ctype>] : noninteractive) . . pq of an fp group
##
## <#GAPDoc Label="PQuotient">
## <ManSection>
## <Func Name="PQuotient" Arg='F, p[, c][, logord][, ctype]'/>
## <Func Name="PQuotient" Arg='F, p[, c][, logord][, ctype] : noninteractive'/>
##
## <Description>
## computes a factor <A>p</A>-group of a finitely presented group <A>F</A>
Expand Down Expand Up @@ -61,10 +61,11 @@ DeclareGlobalFunction( "AbelianPQuotient" );
## most <M>p^{256}</M>. If the parameter <A>logord</A> is present, it will
## compute with factor groups of order at most <M>p^{<A>logord</A>}</M>.
## If this parameter is specified, then the parameter <A>c</A> must also be
## given. The present
## implementation produces an error message if the order of a
## <M>p</M>-quotient exceeds <M>p^{256}</M> or <M>p^{<A>logord</A>}</M>,
## respectively.
## given. If the order of a <M>p</M>-quotient exceeds <M>p^{256}</M>
Copy link
Copy Markdown
Member

@fingolfin fingolfin Oct 15, 2024

Choose a reason for hiding this comment

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

Is 256 an additional limit, or just the default value for logord ?

Copy link
Copy Markdown
Contributor Author

@FriedrichRober FriedrichRober Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it is the default value, not an additional limit (at least what I can tell from the code and how it behaves on input groups whose quotients exceed 256 generators). Probably we should rephrase it. For now, I tried to stay as close to the original documentation as possible

Copy link
Copy Markdown
Contributor Author

@FriedrichRober FriedrichRober Nov 12, 2024

Choose a reason for hiding this comment

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

I changed the documentation now

## or <M>p^{<A>logord</A>}</M>,
## respectively, the behaviour of the algorithm depends on the option
## <A>noninteractive</A>: if it is present, the current implementation
## produces an error message; otherwise it returns <C>fail</C>.
Comment on lines +67 to +68
Copy link
Copy Markdown
Member

@fingolfin fingolfin Mar 28, 2025

Choose a reason for hiding this comment

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

Isn't this the reverse of what the code does? If noninteractive is set (to true), then it returns fail; if it is not set or if it is set to false, then raise an error?

## Note that the order of intermediate <M>p</M>-groups may be larger than
## the final order of a <M>p</M>-quotient.
## <P/>
Expand Down
27 changes: 24 additions & 3 deletions lib/pquot.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1310,7 +1310,9 @@ end );

#############################################################################
##
#F AbelianPQuotient . . . . . . . . . . . initialize an abelian p-quotient
#F AbelianPQuotient . . . . . . try to initialize an abelian p-quotient
## . . . . . . return true if we are sucessful
## . . . . . . return false otherwise
Comment thread
FriedrichRober marked this conversation as resolved.
Outdated
##
InstallGlobalFunction( AbelianPQuotient,
function( qs )
Expand Down Expand Up @@ -1342,6 +1344,9 @@ function( qs )
generators := DifferenceLists( [1..n], trailers );

## Their images are the first d generators.
if Length(gens) < d then
return false;
fi;
qs!.images{ generators } := gens{[1..d]};

## Fix their definitions.
Expand All @@ -1367,6 +1372,8 @@ function( qs )
qs!.collector![SCP_WEIGHTS]{[1..qs!.numberOfGenerators]} :=
[1..qs!.numberOfGenerators] * 0 + 1;

return true;

end );

#############################################################################
Expand All @@ -1376,7 +1383,8 @@ end );
InstallGlobalFunction( PQuotient,
function( arg )

local G, p, cl, ngens, collector, qs, t,noninteractive;
local G, p, cl, ngens, collector, qs, t,noninteractive,
isAbelianPQuotientSucessful;


## First we parse the arguments to this function
Expand Down Expand Up @@ -1453,7 +1461,20 @@ function( arg )
LengthOfDescendingSeries(qs)+1, " quotient" );

t := Runtime();
AbelianPQuotient( qs );
isAbelianPQuotientSucessful := AbelianPQuotient( qs );
if not isAbelianPQuotientSucessful then
if noninteractive then
Copy link
Copy Markdown
Member

@fingolfin fingolfin Mar 28, 2025

Choose a reason for hiding this comment

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

You need to initialize noninteractive before using it.

To use GAP options, use the ValueOption helper. E.g.

Suggested change
if noninteractive then
if ValueOption("noninteractive") = true then

return fail;
else
Error( "Collector not large enough ",
"to define generators for abelian p-quotient.\n",
"To return the current quotient (of class ",
LengthOfDescendingSeries(qs), ") type `return;' ",
"and `quit;' otherwise.\n" );

return qs;
fi;
fi;

Info( InfoQuotientSystem, 1, " rank of this layer: ",
RanksOfDescendingSeries(qs)[LengthOfDescendingSeries(qs)],
Expand Down
16 changes: 16 additions & 0 deletions tst/testbugfix/2024-10-15-PQuotient.tst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# see <https://github.com/gap-system/gap/issues/5809>
gap> F := FreeGroup(["a", "b"]);;
gap> a := F.1;;
gap> b := F.1;;
gap> p := 5;;
gap> G := F / [a^p, b^p, Comm(a,b)];;
gap> PQuotient(G, p, 1, 2 : noninteractive) <> fail;
true
gap> PQuotient(G, p, 1, 1 : noninteractive) = fail;
true

#
gap> PQuotient( FreeGroup(2), 5, 10, 520 : noninteractive ) <> fail;
true
Copy link
Copy Markdown
Member

@fingolfin fingolfin Oct 15, 2024

Choose a reason for hiding this comment

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

actually gives false (hence CI fails)

Copy link
Copy Markdown
Contributor Author

@FriedrichRober FriedrichRober Oct 15, 2024

Choose a reason for hiding this comment

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

Yes, this is something I discuss right now with Hulpke in the original issue thread. The function does not behave like I would expect from the documentation. I already added this test case here, because in my opinion the function should behave like this. So in this case, the quotient can be generated by 520 elements, but the algorithm first needs to create a covering group that exceed this limit. Thus it currently throws fail. But actually I would expect it to not throw fail here.

Copy link
Copy Markdown
Contributor Author

@FriedrichRober FriedrichRober Nov 12, 2024

Choose a reason for hiding this comment

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

I have put these lines out for now. I will open a new issue for this, since it is not clear if there is an easy fix for this.

#
gap> PQuotient( FreeGroup(2), 5, 10, 520 : noninteractive ) <> fail;
true
gap> PQuotient( FreeGroup(2), 5, 10, 519 : noninteractive ) = fail;
true

gap> gPQuotient( FreeGroup(2), 5, 10, 519 : noninteractive ) = fail;
Comment thread
FriedrichRober marked this conversation as resolved.
Outdated
true