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

Jump to bottom

Fix PQuotient error for large groups. #5816

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Fix PQuotient error for large groups. #5816

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 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
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
Member

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
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

## 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>.
## 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 @@

#############################################################################
##
#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
FriedrichRober marked this conversation as resolved.
Show resolved Hide resolved
##
InstallGlobalFunction( AbelianPQuotient,
function( qs )
Expand Down Expand Up @@ -1342,6 +1344,9 @@
generators := DifferenceLists( [1..n], trailers );

## Their images are the first d generators.
if Length(gens) < d then
return false;

Check warning on line 1348 in lib/pquot.gi

View check run for this annotation

Codecov / codecov/patch

lib/pquot.gi#L1348 

Added line #L1348 was not covered by tests
fi;
qs!.images{ generators } := gens{[1..d]};

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

return true;

end );

#############################################################################
Expand All @@ -1376,7 +1383,8 @@
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 @@
LengthOfDescendingSeries(qs)+1, " quotient" );

t := Runtime();
AbelianPQuotient( qs );
isAbelianPQuotientSucessful := AbelianPQuotient( qs );
if not isAbelianPQuotientSucessful then
if noninteractive then
return fail;

Check warning on line 1467 in lib/pquot.gi

View check run for this annotation

Codecov / codecov/patch

lib/pquot.gi#L1467 

Added line #L1467 was not covered by tests
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" );

Check warning on line 1473 in lib/pquot.gi

View check run for this annotation

Codecov / codecov/patch

lib/pquot.gi#L1469-L1473 

Added lines #L1469 - L1473 were not covered by tests

return qs;
fi;

Check warning on line 1476 in lib/pquot.gi

View check run for this annotation

Codecov / codecov/patch

lib/pquot.gi#L1475-L1476 

Added lines #L1475 - L1476 were not covered by tests
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
Member

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
Contributor Author

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.

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