Enable ImmediateVerification and make it useable #280
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #280 +/- ##
==========================================
- Coverage 77.95% 77.94% -0.01%
==========================================
Files 43 43
Lines 18401 18427 +26
==========================================
+ Hits 14344 14363 +19
- Misses 4057 4064 +7
|
Collaborator
Author
|
Ok, so I added setting immediate verification in the This somewhat fixes the first example in #37: I ran this on my machine thrice until it stopped. Without setting immediate verification in With setting the number of random elements immediate verification uses to 10, I have three processes which ran 3000 successfull recognitions and which are still at it. I'll have to check whether this slows down the test-suite by a lot, but that would surprise me. Update: all three failed after roughly 5000 runs. I also tested this once without the "fix immediate verification" commit which also worked fine. Maybe we don't even need that? I have to sleep over that. Also I'm sure this will take care of #35 and the other example in #37. |
Collaborator
Author
|
This also fixes #16 in that I could do 1500 and 7000 successfull recognitions once I added immediate verification to the method |
Collaborator
Author
|
It might also make sense to always do an immediate verification when the kernel generators are created randomly. |
This comment has been minimized.
This comment has been minimized.
Collaborator
Author
|
I also got a fix for #11, but I need to think about that a little more. I'm pushing my WIP, that needs to be cleaned up though. |
ssiccha
force-pushed
the
ss/RandomElm
branch
2 times, most recently
from
54870c2 to
f3782d1
Compare
Collaborator
Author
|
I've converted this into a draft again since I also rewrote the |
ssiccha
force-pushed
the
ss/RandomElm
branch
2 times, most recently
from
b573e6f to
99c4771
Compare
Collaborator
Author
|
The newest commits are due to me trying to find the reason behind an error I'm trying to fix. I've moved the test case producing the error into |
ssiccha
force-pushed
the
ss/RandomElm
branch
5 times, most recently
from
9b60344 to
632f2d9
Compare
Collaborator
Author
|
This is now ready for review. On my machine I have the feeling, that the test suite got so big, that we start hitting random errors regularly.. |
ssiccha
changed the title
fingolfin
reviewed
Aug 18, 2021
Collaborator
Author
|
Rebased onto master. |
Member
|
I do not understand yet how this removes the the need for mandarins, but we can discuss this via Jitsi on Wednesday |
ssiccha
force-pushed
the
ss/RandomElm
branch
3 times, most recently
from
3f54eeb to
1a7f8d9
Compare
Collaborator
Author
|
Tests now pass. Do you think this can be merged as it is @fingolfin ? |
Member
|
I'd like to know how this affects performance. Do the tests get noticeably slower / faster? Even if they are slower, we may want to merge it, but whatever happens should be tested and documented here. As I explained on a conference call 1-2 weeks ago, I am unconvinced this is a proper replacement for Mandarins -- I am in particular concerned that getting to a confidence level similar to what e.g. 100 mandarins provide, one would have to create soooo many random elements with this method (growing rapidly with the number of branch points) that it would get very slow. But I might be wrong! But that's why I am also asking about performance |
fingolfin
reviewed
Sep 6, 2021
During immediate verification, if we encounter a kernel which is marked as trivial, we need to verify that the computed SLP indeed yields the unit of the group. Add a hack for nodes recognised by BlocksModScalars, since then we can't use ri!.isone.
It errored when its second argument was an empty list.
ssiccha
force-pushed
the
ss/RandomElm
branch
3 times, most recently
from
2df019a to
b61be0b
Compare
Add standard argument for FindKernelFastNormalClosure.
Rewrite FindKernelRandom & immediate verification Enable immediate verification for: - GoProjective - NonTransitive - BlockDiagonal Use more random elements in immediate verification Add a helper function `GenerateRandomKernelElementsAndOptionallyVerifyThem`, which is used both when computing random generators of a kernel and when doing immediate verification of a kernel. Add the function `ImmediateVerification`. Slightly change the return values of FindKernelRandom and FindKernelFastNormalClosure: previously `true` denoted success and `false` denoted that something went wrong while computing an SLP in the image node. Now in the latter situation `fail` is returned. This is to bring them in line with the return values of the new function `ImmediateVerification`, which returns `true` upon successfull verification, `false` upon finding new kernel generators, and `fail` upon failure to compute an SLP in the image node. Add a function handling the situation where immediate verification should be performed, but only trivial kernel generators were found. Also remove the `InfoRecog` progress indicator during immediate verification. Reenable and add tests in tst/bugfix.tst and tst/working/veryslow/ClassicalNatural.tst for: Fixes gap-packages#11, fixes gap-packages#16, fixes gap-packages#35, fixes gap-packages#37. squash
ssiccha
force-pushed
the
ss/RandomElm
branch
from
b61be0b to
c59a538
Compare
Collaborator
Author
|
Hrmpf, I cleaned up the handling of the case where the kernel was recognized as being trivial, but now |
Collaborator
Author
|
So, I've compared how often recog incorrectly recognizes the groups in |
FriedrichRober
approved these changes
Sep 8, 2021
Collaborator
Author
With this PR General performance doesn't seem to be affected too much by this PR.
Test runsWith PRAllQuickMasterAllQuick |
Member
|
And what exactly in bugfix.tst became so slow, and why? |
Collaborator
Author
|
If I remove the tests for the above issues, then the test file again runs in 220ms. So everything is fine there. |
Collaborator
Author
|
I added a test for GL(3,27). Maybe then we don't hit the spurious random error anymore. |
Member
|
So because you added some slow tests to |
Collaborator
Author
I think previously there only were two tests in there. I can split the file up, but I'll do that now in a separate PR. I have been fighting with the test suite for so long that I just want to have this in now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enables immediate verification and fixes the following issues:
Fixes #11, fixes #16, fixes #35, fixes #37.
To make the immediate verification work a few fixes to other functions are needed. The main commit is f45c7e0.
Previously this PR also did some other things. I've put these other things into separate PRs such that this one now only contains the commits related to immediate verification.
If we merge this, I think we won't need mandarins anymore. If we want to have the "generate a hundred random elements and send them through the recognition tree" we could do the following: we could add a "thorough verification" option which does what immediate verification does for the root of a recog tree, but with more random elements. If that fails maybe restart the whole computation and if that also doesn't do the trick, tell the user.