Skip to content

Make RECOG.IsThisSL2Natural more precise #487

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

Merged
fingolfin merged 4 commits into from
May 28, 2026
Merged

Make RECOG.IsThisSL2Natural more precise #487

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f057210
Test a stronger condition
SoongNoonien May 26, 2026
8b24b8b
First normalize determinants
SoongNoonien May 27, 2026
5e5be86
Apply suggestion from @fingolfin
SoongNoonien May 27, 2026
b1e88ef
Update a comment
SoongNoonien May 27, 2026
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
24 changes: 14 additions & 10 deletions gap/projective/classicalnatural.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,14 +647,11 @@ RECOG.IsThisSL2Natural := function(gens,f)
q := Size(f);
p := Characteristic(f);
# For small q, compute the order of the group via a stabilizer chain.
# Note that at this point we are usually working projective, and thus
# scalars are factored out "implicitly". Thus the generators we are
# looking at may generate a group which only contains SL2 as a subgroup.
if q <= 11 then # this could be increased if needed
Info(InfoRecog,4,"SL2: Computing stabiliser chain.");
S := StabilizerChain(Group(gens));
Info(InfoRecog,4,"SL2: size is ",Size(S));
return Size(S) mod (q*(q-1)*(q+1)) = 0;
return Size(S) = (q*(q-1)*(q+1));

@fingolfin fingolfin May 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for starting this experiment.

So now we know: With this change we get a bunch of errors in tests related to the naming / recognition of $(P)SL(2,q)$:

testing: /tmp/gaproot/pkg/recog/tst/working/veryslow/ClassicalNatural.tst
########> Diff in /tmp/gaproot/pkg/recog/tst/working/veryslow/ClassicalNatural.tst:60
# Input is:
TestRecogGL(2,4);;
# Expected output:
Stamp: ClassicalNatural
# But found:
Stamp: LowIndex
########
########> Diff in /tmp/gaproot/pkg/recog/tst/working/veryslow/ClassicalNatural.tst:86
# Input is:
TestRecogGL(2,5);;
# Expected output:
Stamp: ClassicalNatural
# But found:
Stamp: LowIndex
########
########> Diff in /tmp/gaproot/pkg/recog/tst/working/veryslow/ClassicalNatural.tst:112
# Input is:
TestRecogGL(2,8);;
# Expected output:
Stamp: ClassicalNatural
# But found:
Stamp: StabilizerChainProj
########
########> Diff in /tmp/gaproot/pkg/recog/tst/working/veryslow/ClassicalNatural.tst:138
# Input is:
TestRecogGL(2,9);;
# Expected output:
Stamp: ClassicalNatural
# But found:
Stamp: StabilizerChainProj
########
########> Diff in /tmp/gaproot/pkg/recog/tst/working/veryslow/ClassicalNatural.tst:154
# Input is:
TestRecogGL(10,9);;
# Expected output:
Stamp: ClassicalNatural
# But found:
ALARM: set count to -1 to skip test!
...

So at least with the current code, we need this.

But it may still be that it only papers over a symptom instead of fixing a root cause. Note that the code originally had the stricter = test and I weakened it in 7bd8018

Thing is, the name of this function strongly suggests it is meant to detect the matrix group $SL_2$; but this code and some other comments it is meant to detect $PSL_2$. This discrepency should be resolved, one way or the other.

@fingolfin fingolfin May 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking at the (currently) only caller for RECOG.IsThisSL2Natural, we see this:

  # First check whether we are applicable:
  if d = 2 then
      if not RECOG.IsThisSL2Natural(GeneratorsOfGroup(g),f) then
          Info(InfoRecog,2,"ClassicalNatural: Is not PSL_2.");
          return TemporaryFailure; # FIXME: TemporaryFailure here really correct?
      fi;
  else
      classical := RecogniseClassical(g);
      if classical.isSLContained <> true then
          Info(InfoRecog,2,"ClassicalNatural: Is not PSL.");
          return TemporaryFailure; # FIXME: TemporaryFailure here really correct?
      fi;
  fi;

  # Now get rid of nasty determinants:
  gens := ShallowCopy(GeneratorsOfGroup(g));
  changed := false;
  gcd := Gcdex(d,q-1);
  for i in [1..Length(gens)] do
      det := DeterminantMat(gens[i]);
      if not IsOne(det) then
          root := RootFFE(f, det^-1, d);
          if root = fail then
              Info(InfoRecog,2,
                   "ClassicalNatural: determinant cannot be normalized in field.");
              return fail;
          fi;
          gens[i] := gens[i] * root;
          changed := true;
      fi;
  od;

So: we first call RECOG.IsThisSL2Natural or RecogniseClassical, and only if those give a "green light' do we proceed to adjust the matrices to have "nice" determinant.

So one alternate fix (which might allow merging this PR here) could be to interchange these two code blocks, and "normalize" the matrices first.

(This PR should not be merged in its current form, though: at the very least, some of the code comments added in 7bd8018 should be adjusted accordingly, or be removed again)

@SoongNoonien SoongNoonien May 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I tried to reorder these two blocks and we'll see if it works now.

fi;

seenqp1 := false;
Expand Down Expand Up @@ -854,12 +851,10 @@ function(ri)
RECOG.SetPseudoRandomStamp(g,"ClassicalNatural");

# First check whether we are applicable:
if d = 2 then
if not RECOG.IsThisSL2Natural(GeneratorsOfGroup(g),f) then
Info(InfoRecog,2,"ClassicalNatural: Is not PSL_2.");
return TemporaryFailure; # FIXME: TemporaryFailure here really correct?
fi;
else
# check if this group contains SL_d -- for d > 2 we can use RecogniseClassical;
# for d = 2, we can use RECOG.IsThisSL2Natural, but only after adjusting
# the determinants of the generators (comes next)
if d <> 2 then
Comment thread
SoongNoonien marked this conversation as resolved.
classical := RecogniseClassical(g);
if classical.isSLContained <> true then
Info(InfoRecog,2,"ClassicalNatural: Is not PSL.");
Expand All @@ -884,6 +879,15 @@ function(ri)
changed := true;
fi;
od;

# Now check whether the normalized matrices generate an SL_2:
if d = 2 then
if not RECOG.IsThisSL2Natural(gens,f) then
Info(InfoRecog,2,"ClassicalNatural: Is not PSL_2.");
return TemporaryFailure; # FIXME: TemporaryFailure here really correct?
fi;
fi;

if changed then
gm := GroupWithMemory(gens);
pr := ProductReplacer(GeneratorsOfGroup(gm),rec(maxdepth := 500));
Expand Down