-
Notifications
You must be signed in to change notification settings - Fork 19
Implement "recognize An/Sn of unknown degree" algorithm by Jambor et al. #176
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.
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
Implement "recognize An/Sn of unknown degree" algorithm by Jambor et al. #176
Conversation
123a0c9 to
04030d0
Compare
015657a to
80a2d14
Compare
fingolfin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice progress, folks :-)
8efeca3 to
4561c2b
Compare
fingolfin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you once again for working on this!
I left some nitpicking comments, as my brain was/is too tired for more. I'll try to look with a more mathematical view in the coming days.
eb0bc5f to
61f0b88
Compare
Codecov Report
@@ Coverage Diff @@
## master #176 +/- ##
==========================================
+ Coverage 78.00% 78.26% +0.26%
==========================================
Files 39 41 +2
Lines 17480 18488 +1008
==========================================
+ Hits 13635 14470 +835
- Misses 3845 4018 +173
|
2fc31df to
ad17602
Compare
|
Thanks for the suggestions! |
39a1019 to
5de20f4
Compare
983a83a to
db1ae05
Compare
64e2368 to
9023de9
Compare
|
In #28 I added an example or test (I don't remember which one it was) which we should be able to recognize efficiently with the unknown degree method. I'm also not so sure anymore whether we should put the "unknown degree recognition" into the projective method databases or whether we should call it from inside recognition of classical or almost simple groups. According to #17 the "known degree recognition" was hooked up previously in For permutation groups it might make sense to put it into the method database. |
645b980 to
966cb30
Compare
Calculate the bound N for permutation groups. Add a reference to Liebeck's "On minimal degrees and base sizes of primitive permutation groups".
The degree n is now inserted into isoData in `ConstructSnAnIsomorphism` instead of `RecogniseSnAn`.
to make the test suite finish quicker.
We do a lot of checks with words in c and r. StripMemory reduces the overhead produced by managing SLPs in these computation. This reduces significantly the computation time in `BolsteringElements`.
Add comments to indicate our requirements for the degree n.
- Change input of recognition method `SnAnUnknownDegree` to `(ri, G)`. - Remove degrees smaller than `11` from tests, since they do not work currently. - Fix SLP reordering. - Rename `isoData` to `recogData` in `RecogniseSnAn`.
A three cycle candidate `x` must satisfy `x ^ 3 = 1`. Obvious, but we missed it. This speeds up the function a lot.
0ea7268 to
2a02215
Compare
|
@fingolfin We put a backup of the squashed version of our branch into this branch on my fork. We now pushed a version of this branch which differs from the squashed one only by having several smaller commits instead of the huge These TODOs are left before this can be reviewed:
|
|
We also want to take a look if we can reduce the amount of iterations in |
2a02215 to
068e69d
Compare
|
Actually, I think, except for the tests, all of mine and @FriedrichRober's points from the previous two posts can be addressed in further PRs. At the moment, testing With respect to the tests: coverage for IMO this can be reviewed as it is now, especially since for me this is blocking some of the other PRs. I'd like to keep most of the commits in this PR to be able to pass the tests if they were checked out. So, @fingolfin could you have another look? |
This is work in progress by @FriedrichRober and me.
Fixes #174.
Fixes #29.