-
Notifications
You must be signed in to change notification settings - Fork 19
Recog An/Sn unknown degree: conder, holt #265
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
base: master
Are you sure you want to change the base?
Recog An/Sn unknown degree: conder, holt #265
Conversation
5ae5367 to
d40a988
Compare
|
Rebased the last commit onto the current version of #176. |
Codecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
d40a988 to
dc4a895
Compare
dc4a895 to
f162f36
Compare
cca25fd to
35a39de
Compare
|
Link to our Hackmd |
96f9a25 to
705504c
Compare
e63f4a8 to
956884d
Compare
|
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: And get the tests to pass again. :) And in Make CallMethods directly write into the RecogNode update the docs of 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. |
3b12114 to
daceec3
Compare
|
Awesome, thank you! |
|
@FriedrichRober do you think you can finish this on your own, now that @ssiccha is gone? Perhaps I can help a bit? |
|
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. |
d45adbb to
7ba5ec6
Compare
85ca56f to
deb2974
Compare
|
@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. |
Compute ThreeCycleCandidates in a "lazy" way, as done in Magma Code GetNextThreeCycle.
7925157 to
af0aba4
Compare
|
Getting a lot of errors now of the sort: Any help is appreciated! I guess we are missing some requirement for the recog tree because the method SnAnUnknownDegree seems to work correctly when testing it manually on the erroring groups. However, instead of we often get in the (projective) matrix group case So this is probably a technical error of inserting our method into the database. |
|
Oh, I messed up the rebase. I will fix this, and this should solve the error |
e6f5836 to
18b7a18
Compare
|
The quick test files seem to work as fast as on master on my machine. I am testing now the slow testsuite, since this is where we encoutered problems with Sergio |
|
The slow test suite takes on master almost 5 minutes and on this branch almost 7 minutes |
…ithm, and highlight the adjustments made to the original algorithm.
|
@fingolfin , I think I fixed the performance problem for most non-AnSn examples now. I changed the iterator for the threecyclecandidates to work in batches and give up very quickly. It remembers in its cache where it stopped, and continues from there if called again. However, the numbers for the batches I chose arbitrarily now. If the specific numbers are a good choice or not, I do not know. So the pull request is close to being finished now |
|
We can see which tests fail after everything has run. I think I remember that some small classical groups were recognised now as alternating or symmetric and therefore destroy the output of some tests. |
|
Exactly, for example GL(4,2) is now recognized as A8, which is correct but not what the test file expects. Edit: this seems to be the only error in the tests |
- deactivate guessdegree code - insert SnAnUnknownDegree after deletedpermutationmodule code in projective.gi
|
@fingolfin, the pull request is ready now. The test suite on my laptop was as quick as on master, however some unrelated errors occures. Two where the stamp is not what was expected in the test file. One where the group was wrongly recognized which would require further investigation. I assume that this is a random error which simply occured from inserting the recognition method into the database. |
|
Fantastic, @FriedrichRober, thank you so much! |
| AUTHOR = {Conder, Jonathan}, | ||
| TITLE = {Algorithms for Permutation Groups}, | ||
| YEAR = {2012}, | ||
| NOTE = {TODO}, |
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.
Could this be filled in by someone? At which university was this thesis written; what kind of thesis was it?
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.
I talked to @aniemeyer and learned that this was a thesis in Auckland, but I didn't find something initially. But now it seems this was indeed a B.Sc. thesis there, and it was written by @jonathan-conder . I have now pinged them, and will also write an email. Perhaps we get permissions to post the thesis somewhere, or perhaps Jonathan is willing to put it on e.g. the arXiv (I am not sure if they accept such things, but it seems to contain original research, and they definitely have at least phd theses?)
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.
Jonathan replied to my email and generously gave permission to share copies of his thesis, and he also made it available at https://jonathan-conder.github.io/afpg.pdf -- this should keep working for the foreseeable future, and I also made sure archive.org has a snapshot. Still, we should probably see if we can also get this onto e.g. Zenodo or so.
Anyway, that gives us enough info to complete this bib record. I'll do so eventually (if nobody beats me to it).
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! I've pushed a bunch of changes (see commit log). I'll try to understand the errors better tomorrow
| # This is usually much smaller than RECOG.SnAnUpperBoundForDegree. | ||
| # The number to compare N with was chosen arbitrarily as a "large" degree. | ||
| # if N > 20 then | ||
| # degreeData := RECOG.GuessSnAnDegree(ri); | ||
| # if degreeData = fail then | ||
| # cache.N := TemporaryFailure; | ||
| # return; | ||
| # fi; | ||
| # N := Minimum(N, degreeData.degree); | ||
| # cache.N := N; | ||
| # fi; |
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.
Is this the only call to RECOG.GuessSnAnDegree?? If so... do we need it at all?
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.
I think we want to keep it just for the sake of Issue #348. But right now the code is not used by SnAnUnknownDegree, and we can surely move it somewhere else.
| # AddMethod(FindHomDbProjective, FindHomMethodsGeneric.SnAnSmallUnknownDegree, 1075); | ||
|
|
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.
This method does not seem to exist?
| # AddMethod(FindHomDbProjective, FindHomMethodsGeneric.SnAnSmallUnknownDegree, 1075); |
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.
Yes, might be an artifact of some old code we wrote. You can safely delete this.
| yTo5 := y ^ 5; | ||
| if not isone(ri)(yTo5) and not isone(ri)(yTo5 * y) then |
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.
Computing
Wouldn't it then be even better to do something like this, which requires either 1, 2 or 3 multiplications ?
| yTo5 := y ^ 5; | |
| if not isone(ri)(yTo5) and not isone(ri)(yTo5 * y) then | |
| y2 := y^2; | |
| if not isone(ri)(y2) then | |
| y3 := y2 * y; | |
| if not isone(ri)(y3) and not isone(ri)(y2 * y3) then |
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.
yes, your suggestion is definitely better
| constants := RECOG.ThreeCycleCandidatesConstants(1. / 4., N); | ||
| for i in [1 .. T] do | ||
| iterator := RECOG.ThreeCycleCandidatesIterator(ri, constants); | ||
| # This is the main method used by RECOG.RecogniseSnAn and RECOG.RecogniseSnAnLazy. |
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.
As far as I can tell, RECOG.RecogniseSnAn is not used anymore. Can we just remove it then, and perhaps rename RECOG.RecogniseSnAnLazy to RECOG.RecogniseSnAn ?
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.
So, in theory RECOG.RecogniseSnAn is the algorithm that you can run with an arbitrary epsilon and it guarantees you to recognise SnAn with this error bound, so it is running all iterations as often as computed in the respective paper. However, in practice it is too expensive to run this method for the recog tree, so we wrote the lazy variant that exits the iterations way sooner. Whether we want to keep it or not depends on whether or not it is interesting to have this version of the algorithm to play around with, which I honestly do not have a strong opinion on. We can definitely rename the functions to RECOG.RecogniseSnAnExact or something like this and then drop the lazy from the other :)
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.
I think we should keep the code, but rename as Friedrich suggested.
The rest of the changes look good to me |
A PR based on #176.
We are going to implement the extensions by Conder and Derek Holt's
GuessAltSymDegree.