Skip to content

Retry kernel recognition for missing generators#519

Open
fingolfin wants to merge 1 commit into
masterfrom
codex/retry-kernel-recognition
Open

Retry kernel recognition for missing generators#519
fingolfin wants to merge 1 commit into
masterfrom
codex/retry-kernel-recognition

Conversation

@fingolfin

Copy link
Copy Markdown
Member

Final verification can expose a missing residual kernel element. Add it to gensN and rerun kernel recognition before aborting.

Co-authored-by: Codex codex@openai.com

Final verification can expose a missing residual kernel element.
Add it to gensN and rerun kernel recognition before aborting.

Co-authored-by: Codex <codex@openai.com>
@fingolfin
fingolfin requested a review from TWiedemann July 14, 2026 11:23
@fingolfin fingolfin added bug Any bug should have this label, even if it also has a more generic label bug: unexpected error A computation runs into an error loop when it should not have labels Jul 14, 2026
@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.51%. Comparing base (6bd8e0a) to head (12654e0).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
gap/base/recognition.gi 75.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #519      +/-   ##
==========================================
- Coverage   86.51%   86.51%   -0.01%     
==========================================
  Files          45       45              
  Lines       19262    19284      +22     
==========================================
+ Hits        16665    16684      +19     
- Misses       2597     2600       +3     
Files with missing lines Coverage Δ
gap/base/recognition.gi 72.91% <75.00%> (+0.38%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread gap/base/recognition.gi
for i in [1..Length(GeneratorsOfGroup( H ))] do
h := GeneratorsOfGroup( H )[i];
if SLPforElement(ri, h) = fail then
x := ImageElm(Homom(ri), h);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hrm, I guess if this returns fail at this point, we are in deep troubles, and ought to abort. Actually, if we did everything right before getting here, shouldn't this be effectively impossible? Hrm.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think x could potentially be fail if KernelRecogNode(ImageRecogNode(ri)) is too small. In any case, I agree that we ought to abort in this situation (which is what the code currently does).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Actually, my statement that the code currently aborts in this situation is not entirely correct. I think it does not abort in the following situation:

  • The for loop finds some h with SLPforElement(ri, h) = fail, and x := ImageElm(Homom(ri), h) is not fail for this h, and s := SLPforElement(ImageRecogNode(ri), x) is also not fail. This results in done being set to false.
  • In a later iteration of the for loop, we find some h with SLPforElement(ri, h) = fail, but this time x := ImageElm(Homom(ri), h) is fail. Since done is still false from a previous iteration, the code does not abort.

@TWiedemann
TWiedemann self-requested a review July 16, 2026 07:44
Comment thread gap/base/recognition.gi
Comment on lines +708 to +734
if x <> fail then
s := SLPforElement(ImageRecogNode(ri), x);
else
s := fail;
fi;
if s <> fail then
y := ResultOfStraightLineProgram(s, ri!.pregensfacwithmem);
z := ri!.gensHmem[i] * y^-1;
if not isone(ri)(z) and
not ForAny(gensN(ri), x -> isequal(ri)(x, z)) then
Add(gensN(ri), z);
Info(InfoRecog, 2,
"Final verification found extra kernel ",
"element (depth=", depth, ").");
done := false;
fi;
fi;
if done then
# NiceGens(ri) is too small, abort
if InfoLevel(InfoRecog) = 1 and depth = 0 then
Print("\n");
fi;
Info(InfoRecog, 1,
"RecogNode <ri> could not be recognised,",
" IsReady(<ri>) is not set, recognition aborts");
return ri;
fi;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if x <> fail then
s := SLPforElement(ImageRecogNode(ri), x);
else
s := fail;
fi;
if s <> fail then
y := ResultOfStraightLineProgram(s, ri!.pregensfacwithmem);
z := ri!.gensHmem[i] * y^-1;
if not isone(ri)(z) and
not ForAny(gensN(ri), x -> isequal(ri)(x, z)) then
Add(gensN(ri), z);
Info(InfoRecog, 2,
"Final verification found extra kernel ",
"element (depth=", depth, ").");
done := false;
fi;
fi;
if done then
# NiceGens(ri) is too small, abort
if InfoLevel(InfoRecog) = 1 and depth = 0 then
Print("\n");
fi;
Info(InfoRecog, 1,
"RecogNode <ri> could not be recognised,",
" IsReady(<ri>) is not set, recognition aborts");
return ri;
fi;
abort := false;
if x <> fail then
s := SLPforElement(ImageRecogNode(ri), x);
else
s := fail;
abort := true;
fi;
if s <> fail then
y := ResultOfStraightLineProgram(s, ri!.pregensfacwithmem);
z := ri!.gensHmem[i] * y^-1;
if not isone(ri)(z) and
not ForAny(gensN(ri), x -> isequal(ri)(x, z)) then
Add(gensN(ri), z);
Info(InfoRecog, 2,
"Final verification found extra kernel ",
"element (depth=", depth, ").");
done := false;
else
# This should not be possible: If z is one or already in gensN(ri),
# then we should have been able to find an SLP for h in the first place.
abort := true;
fi;
fi;
if abort then
# NiceGens(ri) is too small, abort
if InfoLevel(InfoRecog) = 1 and depth = 0 then
Print("\n");
fi;
Info(InfoRecog, 1,
"RecogNode <ri> could not be recognised,",
" IsReady(<ri>) is not set, recognition aborts");
return ri;
fi;

I think this change makes the code abort as it should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug: unexpected error A computation runs into an error loop when it should not have bug Any bug should have this label, even if it also has a more generic label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants