Add constructive recognition for SL_n, take 2 by fingolfin · Pull Request #371 · gap-packages/recog

fingolfin · 2026-02-03T09:34:00Z

This is a successor for PR #330. The only difference for now is that the branch is now hosted in this repo, not in @danielrademacher's branch. This will make it easier for us here to work on it together.

The plan is to further reduce diffs between this and master, and then get it merged.

There will also soon be another PR / branch with the rest of Daniel's code.

TODOs for merging this:

no merge conflicts
all tests must pass
understand the change in gap/projective/classicalnatural.gi‎ and possibly remove or adjust it (see also PR Make RECOG.IsThisSL2Natural more precise #487)
ensure the code added here has good code coverage (i.e., is actually used)
- we can keep some "dead" code if there is a good reason, but then it should be commented accordingly, otherwise it'll lead to a lot of confusion
- if there are no immediate plans to use the dead code, we should consider instead putting it into a contrib subdir (as part of this PR)
generally perform a code review and make sure we understand everything
look out for bottlenecks and potential optimizations (but: correctness over performance, always)

Make sure that the correct path of `regen_doc.g` is used when PackageManager builds the documentation.

preliminary fix in `makedoc.g`

If one wants to build the documentation with version 1.4.3 of PackageManager then also the calls of `OutputTextFile` require paths relative to `DirectoryCurrent()`.

... and add some comments (pointing out this needs documentation, and also an obvious bug in the code that would lead to an unexpected error if the code it is in was ever run)

... which made it depend on the characteristic. This corresponds to a change in our manuscript where it turned out we don't need this, and experiments validate this.

The GAP kernel already does that and this is way faster.

It is from Alnuth, and trivial to reimplement

codecov · 2026-02-03T11:03:18Z

Codecov Report

❌ Patch coverage is 32.00908% with 1797 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.55%. Comparing base (67ae22b) to head (2c1fb00).

Files with missing lines	Patch %	Lines
...projective/constructive_recognition/utils/utils.gi	24.53%	846 Missing ⚠️
...ective/constructive_recognition/SL/sl2_BlackBox.gi	8.91%	327 Missing ⚠️
...rojective/constructive_recognition/SL/GoingDown.gi	33.33%	312 Missing ⚠️
...projective/constructive_recognition/SL/BaseCase.gi	4.72%	242 Missing ⚠️
gap/projective/constructive_recognition/SL/main.gi	52.81%	67 Missing ⚠️
.../projective/constructive_recognition/SL/GoingUp.gi	98.98%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #371      +/-   ##
==========================================
- Coverage   88.13%   81.55%   -6.58%     
==========================================
  Files          45       50       +5     
  Lines       19023    21097    +2074     
==========================================
+ Hits        16765    17205     +440     
- Misses       2258     3892    +1634

Files with missing lines	Coverage Δ
gap/base/kernel.gi	`88.33% <100.00%> (+0.19%)`	⬆️
.../projective/constructive_recognition/SL/GoingUp.gi	`98.98% <98.98%> (ø)`
gap/projective/constructive_recognition/SL/main.gi	`52.81% <52.81%> (ø)`
...projective/constructive_recognition/SL/BaseCase.gi	`4.72% <4.72%> (ø)`
...rojective/constructive_recognition/SL/GoingDown.gi	`33.33% <33.33%> (ø)`
...ective/constructive_recognition/SL/sl2_BlackBox.gi	`8.91% <8.91%> (ø)`
...projective/constructive_recognition/utils/utils.gi	`24.53% <24.53%> (ø)`

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

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

No offense intended, but Daniel didn't make substantial change here other than deleting or moving code

fingolfin · 2026-03-17T17:22:12Z

Good: tests pass, merge conflicts remove, future merging easier
Bad: test coverage on the new code is less than 27%. Quite a bit will be due to dead code. Should clean that up, and for the user code, increase code coverage.
Ugly: the diff to PR #372 keeps growing :-/

Till-Eisen · 2026-05-19T08:59:29Z

I'd like to help move this forward - what would be most useful to work on next?

fingolfin · 2026-05-26T13:16:17Z

      Info(InfoRecog,4,"SL2: size is ",Size(S));
-      return Size(S) mod (q*(q-1)*(q+1)) = 0;
+      # return Size(S) mod (q*(q-1)*(q+1)) = 0;
+      return Size(S) = (q*(q-1)*(q+1));


This change is fishy. See also #487

I wonder why we need it. But if we do need it, then certainly there must be more; e.g. the code comment a few lines above references this code and thus must be adjusted.

Co-authored-by: Max Horn <max@quendi.de>

fingolfin · 2026-05-26T17:09:53Z

@Till-Eisen I've outlined a couple of things in the PR description that we ought to take care before merging this. I'll add some review comments soon. Happy to talk about this more next Tuesday.

fingolfin · 2026-05-26T17:03:39Z

+
+    factors := PrimeDivisors(q-1);
+    counter := 1;
+    F := GF(q);


For this (and all other future code) we should strive to pass around the field, not the size.

This is key first step towards supporting alternative field implementations (such as those from the StandardFF package).

Though in this case, really the field should be derived from G (i.e. the group G is generated by matrices which have a certain BaseDomain...). On the GAP side we are discussing adding a BaseDomain for matrix groups.

fingolfin · 2026-05-26T17:07:54Z

+        G := GroupWithMemory(G);
+    fi;
+
+    one := One(G);


Better to use IsOne (it used to be that IsOne was super slow for matrices, but this was fixed some years ago)

Suggested change

one := One(G);

fingolfin · 2026-05-26T17:10:10Z

+        # Step 1: Find random element of order q-1
+        # foundEle := false;
+        # while not(foundEle) do
+        #    A := PseudoRandom(G);
+        #    if A^(q-1) = one then
+        #        passed := true;
+        #        for factor in factors do
+        #            if A^((q-1)/factor) = one then
+        #                passed := false;
+        #                break;
+        #            fi;
+        #        od;
+        #        if passed then
+        #            foundEle := true;
+        #        else
+        #            counter := counter + 1;
+        #        fi;
+        #    else
+        #        counter := counter +1;
+        #    fi;
+        #    if counter >= max then
+        #        return fail;
+        #    fi;
+        #od;
+


Any reason to keep this? Otherwise I'd rather delete this.

Suggested change

# Step 1: Find random element of order q-1

# foundEle := false;

# while not(foundEle) do

# A := PseudoRandom(G);

# if A^(q-1) = one then

# passed := true;

# for factor in factors do

# if A^((q-1)/factor) = one then

# passed := false;

# break;

# fi;

# od;

# if passed then

# foundEle := true;

# else

# counter := counter + 1;

# fi;

# else

# counter := counter +1;

# fi;

# if counter >= max then

# return fail;

# fi;

#od;

fingolfin · 2026-05-26T17:10:41Z

+        #    fi;
+        #od;
+
+        # Step 1: Find random element of order q-1 (first improvement based on magma code)


Suggested change

# Step 1: Find random element of order q-1 (first improvement based on magma code)

# Step 1: Find random element of order q-1

I don't mind crediting Magma, but this comment is super vague. If anyone knows what improvements this refers to...?

fingolfin · 2026-05-28T08:15:42Z

+        foundEle := false;
+        while not(foundEle) do
+            A := PseudoRandom(G);
+            o := Order (A);
+            if (o mod (q - 1) = 0) then
+                A := A^(o/(q - 1));
+                foundEle := true;
+            else
+                counter := counter +1;
+            fi;
+            if counter >= max then
+                return fail;
+            fi;
+        od;


can be simplified

Suggested change

foundEle := false;

while not(foundEle) do

A := PseudoRandom(G);

o := Order (A);

if (o mod (q - 1) = 0) then

A := A^(o/(q - 1));

foundEle := true;

else

counter := counter +1;

fi;

if counter >= max then

return fail;

fi;

od;

while true do

A := PseudoRandom(G);

o := Order (A);

if (o mod (q - 1) = 0) then

A := A^(o/(q - 1));

break;

fi;

counter := counter +1;

if counter >= max then

return fail;

fi;

od;

Perhaps we should also use RandomElmOrd or so (see also #237, #462)

fingolfin · 2026-05-28T10:18:40Z

+  repeat
+    s:=r^PseudoRandom(g);
+    nulls:=RECOG.FixspaceMat(s);
+    nullspaces:=VectorSpace(GF(q),nulls);


Same questions/suggestions as above

fingolfin · 2026-05-28T10:20:47Z

+    shortr:=newr{[dim-3..dim]}{[dim-3..dim]};
+    shorts:=news{[dim-3..dim]}{[dim-3..dim]};


I assume this means we are looking at stingray matrices with the "head/body" at the bottom right (instead of top left)?

A comment should stat that, I guess.

fingolfin · 2026-05-28T10:22:43Z

+    readydim4:=false;
+    readydim3:=false;
+    repeat
+       u:=PseudoRandom(h);
+       orderu:=Order(u);
+       if orderu mod ((q^4-1)/(q-1)) = 0 then
+          readydim4:=true;
+       elif Gcd(orderu,(q^2+q+1)/Gcd(3,q-1))>1 then
+          readydim3:=true;
+       fi;
+       if readydim4 = true and readydim3 = true then
+          ready:=true;
+          break;
+       fi;
+       count:=count+1;
+    until count=30;


What's the idea here? Is this meant to be a test ("naming") to verify we have $SL_4$ ? Why is that correct, though?

I also wonder if it is better / cheaper / faster than `RecogniseClassical(h,rec(case:="linear"));

fingolfin · 2026-05-28T10:25:28Z

+
+
+# Find random element s = r^PseudRandom(g) such that <r,s> is isomorphic to SL(4,q)
+# and check whether they are isomorphic


What is meant by "they"? The group <r,s>, and... what?

fingolfin · 2026-05-28T10:27:19Z

+
+#g=SL(d,q), given as a subgroup of SL(dim,q)
+#output: [SL(2,q), and a basis for the 2-dimensional subspace where it acts
+RECOG.SLn_godownfromd:=function(g,q,d,dim)


It seems this is only ever called with d=4?

Is dim really necessary as an argument, can't we get that from DimensionOfMatrixGroup(g)?

Likewise for q, as Size(FieldOfMatrixGroup(g))

Daniel Rademacher and others added 30 commits June 13, 2024 14:59

Include SL Code.

941ccd0

Remove some deleted files from read.g

cf89f14

Add fix for manual crash.

df70f5e

Implement Max remark

7fbadd0

preliminary fix in makedoc.g

e5e2bd7

Make sure that the correct path of `regen_doc.g` is used when PackageManager builds the documentation.

Merge remote-tracking branch 'upstream/master' into merge_master

0ada415

New version and add dependencies

f2b49f5

Merge pull request #1 from ThomasBreuer/TB_install

1aa2941

preliminary fix in `makedoc.g`

typo

6502e3b

simplify

9353440

cleanup

bbb4f70

Debug GenerateRandomKernelElementsAndOptionallyVerifyThem

2616344

Remove some dead (commented out) code

01c2703

Restore some code

3847eb7

fix more places where the current directory is needed

36bbef1

If one wants to build the documentation with version 1.4.3 of PackageManager then also the calls of `OutputTextFile` require paths relative to `DirectoryCurrent()`.

merge master

47b7bfc

Remove unused misc/up.gi

ba933a4

Factor out some duplicate code

47e2dd7

Fix error in Computesl2Subspace

5b50878

... and add some comments (pointing out this needs documentation, and also an obvious bug in the code that would lead to an unexpected error if the code it is in was ever run)

Get rid of GoalDim

d6c5130

Fix bug where FewGensAbelian was only used for perm groups

d5dfadd

GoingUp: remove special case for initial stingray

ea7e6a2

... which made it depend on the characteristic. This corresponds to a change in our manuscript where it turned out we don't need this, and experiments validate this.

Pass field not field size to ConvertToMatrixRep

449b989

When compute a^b don't handle b=1 specially

6c99f9b

The GAP kernel already does that and this is way faster.

Merge branch 'master' into Include_ConstructiveRecognition_SL

c4840ee

Some notes from debugging last year

ab0951e

Merge branch 'master' into Include_ConstructiveRecognition_SL

e87c8f3

Mark unused code

b9b1f68

Merge branch 'master' into Include_ConstructiveRecognition_SL

29b880d

Update PackageInfo.g

37ba22c

Merge branch 'master' into Include_ConstructiveRecognition_SL

5120658

fingolfin mentioned this pull request Feb 3, 2026

Add constructive recognition for SL_n #330

Closed

fingolfin changed the title ~~Add constructive recognition for $SL_n$, take 2~~ Add constructive recognition for SL_n, take 2 Feb 3, 2026

fingolfin mentioned this pull request Feb 3, 2026

Daniels constructive recognition code #372

Draft

fingolfin added 2 commits February 3, 2026 11:47

Replace FieldOfPolynomial

8efdd2a

It is from Alnuth, and trivial to reimplement

Tweak RECOG.CheckPolynomialForSelfConjugate

891fb8c

fingolfin added 3 commits February 10, 2026 11:02

Clarify info message

ac79b85

Rename achieve.gi back to gap/projective/classicalnatural.gi

7a72de1

Reduce diff to master

a41b2dc

fingolfin force-pushed the Include_ConstructiveRecognition_SL branch from c4ddfcb to a41b2dc Compare March 17, 2026 11:48

fingolfin added 2 commits March 17, 2026 14:17

Remove Daniel's name from classicalnatural.gi

ff21653

No offense intended, but Daniel didn't make substantial change here other than deleting or moving code

Merge branch 'master' into Include_ConstructiveRecognition_SL

da2a6db

fingolfin mentioned this pull request Mar 21, 2026

Add constructive recognition of SL(2,q) in natural representation 2.0 #398

Merged

fingolfin closed this Mar 24, 2026

fingolfin reopened this Mar 24, 2026

Merge branch 'master' into Include_ConstructiveRecognition_SL

5899b46

Merge branch 'master' into Include_ConstructiveRecognition_SL

0a2b32c

SoongNoonien mentioned this pull request May 26, 2026

Make RECOG.IsThisSL2Natural more precise #487

Merged

fingolfin commented May 26, 2026

View reviewed changes

Comment thread gap/projective/classicalnatural.gi Outdated

Apply suggestions from code review

2c1fb00

Co-authored-by: Max Horn <max@quendi.de>

fingolfin commented May 28, 2026

View reviewed changes

	# Step 1: Find random element of order q-1 (first improvement based on magma code)
	# Step 1: Find random element of order q-1

		shortr:=newr{[dim-3..dim]}{[dim-3..dim]};
		shorts:=news{[dim-3..dim]}{[dim-3..dim]};



		# Find random element s = r^PseudRandom(g) such that <r,s> is isomorphic to SL(4,q)
		# and check whether they are isomorphic

Conversation

fingolfin commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

fingolfin commented Mar 17, 2026

Uh oh!

Till-Eisen commented May 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fingolfin commented May 26, 2026

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fingolfin commented Feb 3, 2026 •

edited

Loading

codecov Bot commented Feb 3, 2026 •

edited

Loading