Recog An/Sn unknown degree: conder, holt

[ssiccha]

A PR based on #176.

We are going to implement the extensions by Conder and Derek Holt's
GuessAltSymDegree.

[ssiccha]

Rebased the last commit onto the current version of #176.

[codecov]

Codecov Report

❌ Patch coverage is 75.32468% with 95 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.48%. Comparing base (c309d8a) to head (16bace0).

Files with missing lines	Patch %	Lines
gap/generic/SnAnUnknownDegree.gi	75.13%	95 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
+ Coverage   69.12%   70.48%   +1.35%     
==========================================
  Files          43       43              
  Lines       18321    18637     +316     
==========================================
+ Hits        12665    13136     +471     
+ Misses       5656     5501     -155     

Files with missing lines	Coverage Δ
gap/matrix.gi	92.97% <100.00%> (+0.01%)	⬆️
gap/perm.gi	79.76% <100.00%> (+0.07%)	⬆️
gap/projective.gi	83.17% <100.00%> (+0.08%)	⬆️
gap/generic/SnAnUnknownDegree.gi	85.07% <75.13%> (-5.02%)	⬇️

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[FriedrichRober]

Link to our Hackmd

[ssiccha]

We should be almost done. We think there are two TODOs left: clean up the tests and figure out what to do with small classical groups like: Omega(-1,4,3). It has element orders <= 5, thus slips through our heuristics to weed out groups which are not isomorphic to An or Sn. Hence the SnAnUnknownDegree algorithm takes forever until it realizes, that the group in question is not an An or Sn. We think we should be able to alleviate this by simply saying that we're NeverApplicable if the vector space our group acts has less than e.g. 100 elements. And then we'll post a challenge on slack or the GAP forum whether somebody can find a small group, which makes the algorithm run slow.

And get the tests to pass again. :)

And in Make CallMethods directly write into the RecogNode update the docs of CallMethods.

And, @FriedrichRober is writing a short text, how all the parts fit together, that is how our implementation differs from a "vanilla" implementation of the JLNP algorithm according to the paper.

[fingolfin]

Awesome, thank you!

[fingolfin]

@FriedrichRober do you think you can finish this on your own, now that @ssiccha is gone? Perhaps I can help a bit?

[FriedrichRober]

Yes, I think I can finish this on my own. The code is finished (except for handling small classical groups). What is missing now is to finish the documentation (comments) and looking at the runtime of the test suite.

[FriedrichRober]

@fingolfin, ich habe nun die Änderungen implementiert, wie wir es besprochen haben. Wir geben früher auf, und speichern uns die Iteratoren im recog node ab, um sie später weiter zu verwenden. Trotzdem dauert die Test-Suite auf meinem Rechner 3-4 Minuten länger als auf dem master. Meiner Meinung nach ist dieser Zustand inakzeptabel, um das einfach so zu mergen :(

Ich muss dann nochmal genau schauen, an welchen Stellen wir zu viel Zeit verschwenden, und ob sich da was machen lässt.

[TWiedemann]

Suggested change

	# We make a small improvement to the version described in
	# <Cite Key="JLNP13"/>. The order of r ^ M is a 2-power.
	# It can be at most 2 ^ logInt2N. Thus, if we find an r such that
	# (r ^ M) ^ (2 ^ logInt2N) is non-trivial, then we can return
	# NeverApplicable.
	# We make a small improvement to the version described in
	# <Cite Key="JLNP13"/>. The order of r ^ M is a 2-power.
	# It can be at most 2 ^ logInt2N. Thus, if we find an r such that
	# (r ^ M) ^ (2 ^ logInt2N) is non-trivial, then we can return
	# NeverApplicable. I.e. we need one iteration less than in [JLNP13],
	# which also computes (r ^ M) ^ (2 ^ (longInt2N + 1))

Make the difference between the code and JLNP13 more explicit (hoping that I understood this correctly, I am not sure).

[TWiedemann]

Suggested change

	Li := L;
	Li := Minimum(Li, B);
	Ki := Ki + K;
	Ki := Minimum(Ki, C);
	curInvolutionPos := 1;
	return SnAnTryLater;
	elif curInvolutionPos = Li then
	Li := Li + L;
	Li := Minimum(Li, B);
	# Last involution reached. Restart with next batch of conjugates later.
	Li := Minimum(L, B); # Reset Li to initial value
	Ki := Minimum(Ki+K, C); # Try up to K more conjugates later
	curInvolutionPos := 1; # Restart from first involution
	return SnAnTryLater;
	elif curInvolutionPos = Li then
	# End of batch of involutions reached. Try next batch later.
	Li := Minimum(Li+L, B);

The new Minimum statements should be equivalent to the old ones. Also added some comments.

[TWiedemann]

@FriedrichRober I went through ThreeCycleCandidatesIterator and it looks good to me! The only other comment I have is that I think whenever you write "commutating", you actually mean "non-commutating": The algorithm looks for conjugates of the involution t that do not commute with t, and counts how many it has found already. Hence I think nrCommutatingConjugates should be appropriately renamed, and some comments adapted.

[TWiedemann]

This comment explaining/defining the monomorphism Grp(ri) -> S_n given by s and t also applies to the previous function RECOG.FindAnElementMappingIToJ, so I suggest to move it to the general comments that apply to both functions (currently lines 825-828). We could also add a reference to Definition 3.2.2 in Conder's thesis where this notion of "alternating n-generators" is defined.

[TWiedemann]

The implementation of RECOG.FindAnElementMappingIToJ and RECOG.FindImageSnAnSmallDegree looks flawless to me.

[TWiedemann]

Suggested change

	# In <Cite Key="JLNP13"/> filter is called `xi`.
	# In <Cite Key="JLNP13"/> filter is called `X_i`.

Is this correct? I initially thought the comment referred to the Greek letter \xi.

[TWiedemann]

Suggested change

	# Algorithm 4.13 ConstructLongCycle in [JLNP13]
	# ri : recognition node with group G

Add reference. (This function was not changed in this PR, but this seems like a useful addition.)

[TWiedemann]

Suggested change

	# - g: element of G.
	# - k: integer.
	# If G \in [S_n, A_n] and c is a 3-cycle, then with probability at least 1-eps,
	# g is a k-cycle matching c (and also k >= max(3n/4, 9)).

Make statement a bit more precise (again, was not touched in this PR). I think we do not need the lower bound on k, but it is also mentioned in [JLNP].

[TWiedemann]

Suggested change

	# Returns an integer m such that if the group represented by ri is isomorphic to S_n,
	# then necessarily m <= n

[TWiedemann]

Suggested change

	Print("GuessAltsymDegree works only for degree > 6\n");
	Print("GuessSnAnDegree works only for permutation degree > 6 or matrix dimension > 2\n");

[TWiedemann]

Suggested change

	ct := 0;
	# vprintf IsAltsym: "New E, E = %o, O = %o, elt order = %o, Randoms = %o\n", mindege, mindego, o_fact, cte+cto;
	ct := 0;
	# vprintf IsAltsym: "New E, E = %o, O = %o, elt order = %o, Randoms = %o\n", mindege, mindego, o_fact, cte+cto;

Indentation.

[TWiedemann]

Suggested change

	# If mindego > mindege we assume the group is alternating, otherwise
	# If mindego > mindege we guess that the group is alternating, otherwise

I think "guess" is more precise here than "assume".

[TWiedemann]

I find this part confusing. We only exit the preceding loop if mindeg, mindege and mindego did not change for many iterations, and after the loop, ct is the smallest positive integer for which the loop condition
(ct < Maximum(mintries, fac * mindeg * Int(Ceil(Log(Float(mindeg+1))))) or mindego = mindege+1) and ct <= maxtries
fails. Hence I think this if statement is equivalent to the following:

• If mindego = mindege+1, we always return fail (because the loop can only end with ct > maxtries.
• If mindego <> mindege+1, we return fail at this point if and only if fac * mindeg * Int(Ceil(Log(Float(mindeg+1)))) > maxtries. (Here I use the reasonable assumption that mintries < maxtries.)

Is this the desired behaviour? A previous comment says that the function returns fail if "maxtries elements are sampled with no decision made", but this does not seem to match what is happening.

[TWiedemann]

These counters are increased later on, but their values are never used for anything. I think they can be removed.

[TWiedemann]

The guessing strategy of RECOG.GuessSnAnDegree seems reasonable to me. I.e. if it does not fail, the guess seems reasonable. I find it hard to estimate how likely it is to fail.

[TWiedemann]

Suggested change

	if not IsInt(N) then
	return;
	fi;
	# if not IsInt(N) then
	# return;
	# fi;

If the code after these three lines is commented out, then they should also be commented out because they no longer do anything.

[TWiedemann]

Suggested change

	if not IsInt(N) then
	# If N is TemporaryFailure or NeverApplicable, pass it on
	if not IsInt(N) then

[TWiedemann]

I am finished with my review. Everything looks good, so I think we can merge as seen as all comments above are resolved.

I think this PR will resolve #348, but correct me if I am wrong.