Speed up computation of the intersection of group cosets in the generic cases and even more so for permutation groups

[MathieuDutSik]

The intersection of cosets in the gap code is right now inadequate. The cosets are considered as sets which means that we do not have the key information that it is either empty or a closet in the output.

The PR addresses that issue. For general groups we write the intersection as $(H_1\cap H_2 \sigma) s$ and we simply iterate over the right cosets of $H_1$ with respect to $H_1\cap H_2$.

For permutation groups, we do a number of preprocessing tricks in order to simplify and/or resolve the problem. But in the end, we still have to iterate over the cosets. There are possibilities of further improvements, but I think this is a good first step.

#4865

[hulpke]

Thank you. Looks very good to me!

[fingolfin]

Thank you, much appreciated! I did not yet have time to check this out in detail, but it looks promising

Unfortunately most of the new code is not yet covered by our code coverage, which reduces confidence in it a bit. But on the upside, I see you already added tests, which simply are not yet run. So if you can move them as indicated by my comments elsewhere, we can see if that rectifies the issue :-)

[MathieuDutSik]

Regarding checks, I implemented the function as a separate user function before putting it into the cset.gi and csetperm.gi files. I tested it on 10000 pairs of groups chosen at random with about 100 intersections being non-trivial. The obtained cosets considered as sets were compared with the list of permutations being created in the master code.

I put the two cases that showed up when debugging it in testextra (see my other remark) but yes I can put them elsewhere and add more tests as requested.

[fingolfin]

Some more remarks. Still not done reading all, but I have to leave now and figured this is already something

Thank you once more for doing this.

[ChrisJefferson]

I had a quick test comparing this to Vole, and once I fixed some typos (U -> theInt, shift -> x1, LMoved -> listMoved), they seem to produce the same answers.

I tested the generic method by temporarily forcing GAP to use it for rightcosets of permutations by editing the code, I'm not sure of the best way of testing it in general.

[fingolfin]

I took the liberty of rebasing, squashing and fixing things. Let's see how CI fares (modulo the failures in the testpackages jobs, see issue #4878

[ChrisJefferson]

While this isn't how GAP tests are often written, I'd suggest changing this to:

gap> Intersection(...) = RightCoset(..);
true

As that is more resiliant to these annoying types of changes.

[ChrisJefferson]

you don't have to do this, only if it makes it easier for you to write / update tests.

[MathieuDutSik]

ok, will do that.
Maybe we could parse the result of the runs and check them. I had that issue when running C++ vs GAP, because of gap formatting, I had to postprocess the output in order for the comparison to match.

[ChrisJefferson]

Unfortunately, not all objects in GAP can be parsed back in from their printed form, so while that would work here, it wouldn't work in general.

[MathieuDutSik]

I had a quick test comparing this to Vole, and once I fixed some typos (U -> theInt, shift -> x1, LMoved -> listMoved), they seem to produce the same answers.

I tested the generic method by temporarily forcing GAP to use it for rightcosets of permutations by editing the code, I'm not sure of the best way of testing it in general.

Thank you for correcting the typos! Those stylistic correction create a lot of mess.
Besides I did check on 10000 random cases by comparing the intersection of sets and
no error was found.

[MathieuDutSik]

I took the liberty of rebasing, squashing and fixing things. Let's see how CI fares (modulo the failures in the testpackages jobs, see issue #4878

Now, the CI pass. Let me know if something else prevents merging the PR.

[fingolfin]

I have two main concerns with this PR (all the other comments really are optional suggestion):

1. all these repeated computations of closure groups and intersections aren't cheap. I wonder esp. for smaller examples whether they aren't slower than what your "naive" method above does, or even a naive intersection? Perhaps if one of the cosets has less than, say, 50 elements, it would be faster to fall back to a brute force search? As such: did you perform any performance timings to check that this really is an improvement in all cases, or at least does not slow down things too much?
2. Why is all this correct? There is one longish comment explaining some stuff and referencing a paper by Babai. But a ton of "criterion" come before it, but without any explanations as to why they are correct. Granted, it may not be hard to figure this out with some thinking for each. But if we ever need to a fix a bug in this case we'd have to slowly walk through all of this, to understand what's going on. Some comments would do wonders. Or at least some tests that trigger each case -- but right now most (all?) of them are not covered by a test at all, as Codecov shows.

Now, don't get me wrong, I am really grateful you took the effort to prepare this. Also, a lot of existing GAP code does not match this quality criterion (which I think is really bad). But to merge this, I really need to be convinced this (a) an improvement and (b) correct.

Oh, and it also feels a bit to me as if this code is redoing a lot of what Partition backtrack does. As such, I wonder if our partition backtrack code doesn't have a way to find one element of a coset intersection? @ChrisJefferson do you know? Doesn't ferret have such a thing? What about vole?

If I find the time, I'll try to read through more of the code and suggest e.g. possible code comments that I feel would explain things.

[ChrisJefferson]

There is probably any interesting study to be done on the best way to intersect cosets in general.

A slight diversion: One major reason "vole" exists is after written "ferret" (my C++ implementation of partition backtracking), I realised I'd hard-wired various assumptions that made solving coset, and canonical image problems, very hard to implement. It's easy to do cosets if you think about it from the start, but given the lack of documentation of GAP's partition backtracker, I don't really want to dig through it and try to make it do cosets now, if it never did before.

I'm also not sure, in general, the best way of doing cosets. I think it might be an interesting research project -- coset intersection is in practice the problem vole finds hardest, and in some cases this PR is beating vole (in other cases vole is winning, so it's not a clear win one way or the other).

Based on past experience, I would encourage Mathieu, if you want to, to drop more comments in explaining how things work - in general most people never read library code, and it is useful for those who come along 10 years later to know what is going on :)

[fingolfin]

A few more comments.

I also want to explicitly say: @MathieuDutSik I can totally understand if you are fed up by constant "complaints" from me; I hope it is at least clear that they are well-intentioned, even if perhaps annoying...

[ChrisJefferson]

While testing the code I wrote some more tests (I didn't find any bugs) -- in particular the matrix group tests provide complete coverage for the non-perm code. I paste them here, feel free to use them as is, or alter if you want.

# test trivial cases
gap> Intersection(RightCoset(Group([],()), ()), RightCoset(Group([],()), (1,2))) = [];
true
gap> Intersection(RightCoset(Group((1,2,3)), ()), RightCoset(Group((1,2,3)), (1,2))) = [];
true
gap> Intersection(RightCoset(AlternatingGroup(6), ()), RightCoset(AlternatingGroup(6),(1,2))) = [];
true
gap> Intersection(RightCoset(AlternatingGroup([1..5]), (1,2)), RightCoset(AlternatingGroup([6..10]), (1,2))) = RightCoset(Group(()), (1,2));
true

# test intersection non-permutation cosets
gap> matcyc := CyclicGroup(IsMatrixGroup, GF(3), 2);;
gap> [] = Intersection(RightCoset(matcyc, [[0*Z(3), Z(3)], [Z(3), Z(3)^0]]),
>                      RightCoset(matcyc, [[Z(3), Z(3)], [Z(3), 0*Z(3)]] ) );
true
gap> RightCoset(matcyc, [[0*Z(3), Z(3)], [Z(3), Z(3)^0]]) =
>      Intersection(RightCoset(matcyc, [[0*Z(3), Z(3)], [Z(3), Z(3)^0]]),
>                   RightCoset(matcyc, [[Z(3), Z(3)^0], [0*Z(3), Z(3)]] ) );
true
gap> rc1 := RightCoset(Group( [[0*Z(3), Z(3)], [Z(3), 0*Z(3)]], [[Z(3), Z(3)], [Z(3)^0, 0*Z(3)]],
>   [[Z(3), 0*Z(3)], [0*Z(3), Z(3)]] ), [[Z(3), Z(3)^0], [Z(3)^0, Z(3)^0]]);;
gap> rc2 := RightCoset(Group( [[Z(3), Z(3)], [0*Z(3), Z(3)^0]], [[0*Z(3), Z(3)], [Z(3)^0, 0*Z(3)]],
>   [[Z(3), Z(3)], [Z(3), Z(3)^0]], [[Z(3), 0*Z(3)], [0*Z(3), Z(3)]]), [[0*Z(3), Z(3)], [Z(3)^0, Z(3)]]);;
gap> RightCoset(Group( [[Z(3), Z(3)], [0*Z(3), Z(3)^0]], [[Z(3)^0, Z(3)^0], [0*Z(3), Z(3)]] ),[[0*Z(3), Z(3)^0], [Z(3), Z(3)^0]]) =
>      Intersection(rc1, rc2);
true
gap> [] = Intersection(RightCoset(matcyc, One(matcyc)), rc1);
true
gap> RightCoset(Group( [ [ Z(3)^0, 0*Z(3) ], [ 0*Z(3), Z(3)^0 ] ] ),
> [ [ 0*Z(3), Z(3)^0 ], [ Z(3)^0, 0*Z(3) ] ]) = Intersection(RightCoset(matcyc, One(matcyc)), rc2);
true

[MathieuDutSik]

I also want to explicitly say: @MathieuDutSik I can totally understand if you are fed up by constant "complaints" from me; I hope it is at least clear that they are well-intentioned, even if perhaps annoying...

My only issue is about the kind of double standard. A lot of code in GAP would require some upgrade and cleanup.

But I cannot really complain as I wrote something like 20-40 papers using GAP, so really no issue here.

[fingolfin]

Well, I would not accept a lot of the code that is in there right now in its current form, and I wish more professors taught their students about the important of systematic tests, documentation algorithms and describing why they are correct, etc. People rely on systems like GAP to produce correct results and they'd be dismayed if they knew how often they produce garbage. See also kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them

[MathieuDutSik]

Thank you, I inserted those tests.

[MathieuDutSik]

It may be true that there is room to improve on the heuristics, when isn't there room for that?
But I consider this outside of the scope of this PR, at least from my viewpoint.

[fingolfin]

Oh I fully agree.

[fingolfin]

I'll make a few more tweaks (mostly adding some comments) and then will merge.

Thank you very much @MathieuDutSik for your great contribution and your patience -- and sorry for pestering you so much. But I really think the result is even better than what it started out with.

[fingolfin]

Finally found the time to go through this and add the explanatory comments. Also refactored the code a bit more.

I am very happy with this now. Once test pass, we can merge it.

Thanks again!