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.

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?

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

fingolfin · 2026-05-29T08:35:12Z

Another TODO: in Daniel's thesis, the algorithm 26 ComputeHorizontalTransvections currently differs from what @aniemeyer and me manage to prove. It would be good to check if the algorithm as written in the thesis matches the implemented version precisely, or not...

The difference is that in the thesis, in five places entries of the matrix $c^{-1}$ are used. But in our write-up, we instead use entries of $c$ in what corresponds to line 11 of the written algorithm.

fingolfin · 2026-05-29T09:11:07Z

+
+        # Now cleanup row n left of column n:
+        for j in [1..w.n-1] do
+            tf := DoRowOp_n(tf,w.n,j,-c[i,j],w);


It seems this matches the paper draft @aniemeyer are working on, and disagrees with line 11 of Algorithm 26 in the thesis. Phew!

…tive_recognition

fingolfin · 2026-06-08T11:22:59Z

I am realizing more and more that there is very little new code here, mostly existing code was modified.

My plan is now this: first, re-arrange and split the content of sl.gi to match the structure in this PR. Then merge / rebase on that -- then we can better see the actual changes.

Depending on the findings, we may then directly go over to PR #372 and treat it similarly (there, I expect a lot more genuinely new code)

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

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

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

fingolfin commented May 29, 2026

View reviewed changes

SoongNoonien reviewed Jun 1, 2026

View reviewed changes

Comment thread makedoc.g Outdated

SoongNoonien reviewed Jun 1, 2026

View reviewed changes

Comment thread regen_doc.g Outdated

fingolfin force-pushed the Include_ConstructiveRecognition_SL branch from e660caa to 2c1fb00 Compare June 2, 2026 09:15

SoongNoonien and others added 5 commits June 2, 2026 11:20

Remove some doc building changes

4740403

Reset kernel.gi

ea048d4

Reduce diffs between gap/projective/sl.gi and gap/projective/construc…

6ae9b29

…tive_recognition

Merge branch 'master' into Include_ConstructiveRecognition_SL

1b5f910

Use IsOne

b5200a2

fingolfin mentioned this pull request Jun 5, 2026

Add better code for finding SL(2,q) inside SL(4,q) #490

Open

fingolfin mentioned this pull request Jun 23, 2026

Add RECOG.FixspaceMat and RECOG.EigenspaceMat #506

Merged

	# 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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

fingolfin commented Mar 17, 2026

Uh oh!

Till-Eisen commented May 19, 2026

Uh oh!

Uh oh!

Uh oh!

fingolfin commented May 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

fingolfin commented May 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fingolfin commented Jun 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fingolfin commented Feb 3, 2026 •

edited

Loading