Fix bugs in low-index-subgroup functions by stertooy · Pull Request #202 · gap-packages/polycyclic

stertooy · 2026-10-04T21:37:01Z

Closes #201. Builds on top of #209.

AI Disclosure: bugs discovered by GPT-6 Astra during a package audit.

fingolfin · 2026-10-05T16:15:37Z

Has a merge conflict now; maybe merge PR #203 before resolving the merge conflict?

stertooy · 2026-10-05T18:01:41Z

Done!

codecov · 2026-10-05T18:03:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.71%. Comparing base (1f2820b) to head (11db6b0).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
+ Coverage   67.76%   68.71%   +0.95%     
==========================================
  Files          88       88              
  Lines       12914    12913       -1     
==========================================
+ Hits         8751     8873     +122     
+ Misses       4163     4040     -123

Files with missing lines	Coverage Δ
gap/pcpgrp/findex.gi	`96.64% <100.00%> (+4.42%)`	⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

fingolfin · 2026-10-05T21:22:51Z

    # reset n, if the layer is finite, and compute divisors
    if p > 0 then
-        m := Gcd( n, p^l );
+        m := Gcd( n, p );


Why is this correct? I would assume that if we hav a p-layer of length l, it means we can go up to p^l down, so Gcd(n, p^l) seems plausible, at least on the surface? Mind you, without having read or understood the rest of the PR.... I guess either comments or "speaking" variables names would help, I have no idea what m is supposed to be at this point.

Maybe it would be better to split this PR after all, as it is not completely clear to me which bits belong to which bugfix, and also I have different trust levels in different parts

My understanding is this: we're looking for subgroups of $A := G/N$ (first layer) whose index divides $n$, with $A$ either free abelian ($\mathbb{Z}^{\ l}$) or elementary abelian ($C_p^{\ l}$). Any such subgroup contains $nA$, so it suffices to check subgroups of $A/nA \cong C_m^{\ l}$, where $m = n$ in the free abelian case, and either $m = p$ or $m = 1$ in the elementary abelian case, depending on whether $p$ divides $n$ or not.

If my interpretation is correct, I don't see why one would take $\gcd(n,p^l)$. Perhaps the order and exponent were mixed up?

Some comments explaining this would definitely help, I agree.

That does sound plausible, I'll try to have another look at the code tonight to see if I can make sense of it with that idea in mind.

fingolfin · 2026-10-05T21:26:25Z

This goes beyond this PR, but still want to ask: Doesn't this just mean m = 1 ? If so perhaps better to write it as such and move it before the computation of d?

Yep, good point.

This also looks very similar to the two changes I commented on with "Looks good, makes sense" -- perhaps those two can go in their own PR, together with a change here (I imagine the code would look similar in all three places)?

fingolfin · 2026-10-05T21:28:28Z

OK, here it takes about "normed bases in m^l" which could perhaps support the Gcd(n,p) vs. Gcd(n,p^l) bit above.

It also raises new questions, though, like, what does "in m^l" even mean 😂

I interpreted $m^l$ as short for $C_m^{\ l}$.

fingolfin · 2026-10-05T21:31:43Z

        dep := List( e, PositionNonZero );
        ind := List( [1..Length(e)], x -> e[x][dep[x]] );
        ind := Product( ind ) * m^(l - Length(e));
-        if ind <= m then
+        if n mod ind = 0 then


OK, so dep contains the positions of the "lead terms" (pivots), while ind contains the values of said lead terms. Why it is called ind remains a mystery. sigh.

But then we compute the product of these values in ind, and multiply this by a power of m... WHAT? Comments would be sooooooo nice... sigh

I have absolutely no idea whether the change here is correct, neither the old nor the new code make sense to me at this point :-(.

I think ind really stands for "index". This index is the product of the pivots, but some are missing because their vectors are trivial in the quotient $C_m^{\ l}$. Those are exactly the ones with pivot $m$. Those "missing" vectors get added back explicitly later on line 80.

The function returns a list of records, each containing repr, which is a subgroup $H$ s.t. $N \leq H \leq G$ of index ind. And by working on the next layers we still need to find a subgroup $K$ of $H$ of index n / ind, so that $K$ has index $n$ in $G$. But if ind does not divide n, then $K$ must have non-integer index in $H$ which makes no sense.

In particular, the old condition ind <= m also really required m to be either n or p^l, I think, now that m is either n or p this would give incomplete results when not erroring.

fingolfin · 2026-10-05T21:32:55Z

+    if n = 1 then
+        return [rec( repr := G, norm := G, open := 1 )];
+    elif IsTrivial(G) then
+        return [];
+    fi;


Looks good, makes sense

fingolfin · 2026-10-05T21:33:02Z

+    if n = 1 then
+        return [G];
+    elif IsTrivial(G) then
+        return [];
+    fi;
+


Looks good, makes sense

stertooy · 2026-10-06T12:37:00Z

I added some explanations. If those make sense to you, I can add some comments to hopefully make it more clear what the function does.

The two "trivial case handling" changes can easily be split off in a separate PR, if you want. The other two bugs would both need a change in the if ind <= m then line, though.

fingolfin · 2026-10-06T13:10:30Z

I added some explanations. If those make sense to you, I can add some comments to hopefully make it more clear what the function does.

Ah, yes, that sounds like a good idea: what you write does sound sensible to me. So if you add some comments along those lines, I could then read it again, and see if it makes more sense and possible suggest further comment tweaks. Thank you!

stertooy · 2026-10-06T16:12:35Z

I've added (hopefully) more helpful comments. I'll try split off the "trivial cases" changes in a separate PR and then rebase this one on top of that one.

stertooy force-pushed the fix-low-index branch from 730e0e9 to a6dbfae Compare October 5, 2026 18:00

fingolfin reviewed Oct 5, 2026

View reviewed changes

stertooy mentioned this pull request Oct 6, 2026

Fix bugs in low-index-subgroup functions (trivial cases) #209

Merged

stertooy added 2 commits October 6, 2026 22:54

Fixes for trivial cases in LowIndexSubgroups

48295ba

Fix SubgroupsFirstLayerByIndex and add more helpful comments

a40eefa

stertooy force-pushed the fix-low-index branch from efa5d23 to a2247fe Compare October 6, 2026 20:57

stertooy and others added 4 commits October 6, 2026 22:57

Add extra tests for SubgroupsFirstLayerByIndex

a2247fe

Merge branch 'master' into fix-low-index

4353e22

Add changelog entry

5b4744e

Merge branch 'master' into fix-low-index

11db6b0

Conversation

stertooy commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fingolfin commented Oct 5, 2026

Uh oh!

stertooy commented Oct 5, 2026

Uh oh!

codecov Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stertooy Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stertooy Oct 6, 2026 • edited by fingolfin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stertooy commented Oct 6, 2026

Uh oh!

fingolfin commented Oct 6, 2026

Uh oh!

stertooy commented Oct 6, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stertooy commented Oct 4, 2026 •

edited

Loading

codecov Bot commented Oct 5, 2026 •

edited

Loading

stertooy Oct 6, 2026 •

edited

Loading

stertooy Oct 6, 2026 •

edited by fingolfin

Loading