Fix unexpected errors in recog and genss by improving GroupWithMemory when given compressed matrices over small fields
#5907
Conversation
This file contains 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
... at least if the wrapped matrixobj had it set (which really should always be the case). This fixes a nasty long-standing issue in genss / orb / recog where `DefaultScalarDomainOfMatrixList` returned to small a field when called on a list of matrixobj-with-memory. This then caused `GroupWithGenerators` to rewrite the generators over a smaller field, which then lead `genss` to produce (compressed) vectors for use as base points in the orbit algorithm over the smaller field; and a matching hash function. When later generators got added that really needed the larger field, everything exploded in confusing ways.
fingolfin
added
kind: bug
fingolfin
changed the title
GroupWithMemory when the input are compressed matrices over small fields, fixing unexpected errors in recog and genssrecog and genss by improving GroupWithMemory when given compressed matrices over small fields
ThomasBreuer
approved these changes
Jan 17, 2025
There was a problem hiding this comment.
Yes, the wrapped object and the underlying matrix must have the same behaviour.
(In GAP 4.14.0, GroupWithMemory did not work at all in this situation because GroupWithGenerators stripped the memory. This has been fixed just in #5874.)
Co-authored-by: Thomas Breuer <sam@math.rwth-aachen.de>
|
Yes indeed, PR #5874 is a necessary first step in the fix. It took me far too long to understand that, in the past I was looking in the completely wrong places when i tried to understand the issues I was seeing in group recognition :-( |
|
Backported to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
... at least if the wrapped matrixobj had it set (which really should always be the case).
This fixes a nasty long-standing issue in genss / orb / recog where
DefaultScalarDomainOfMatrixListreturned to small a field when called on a list of matrixobj-with-memory. This then causedGroupWithGeneratorsto rewrite the generators over a smaller field, which then leadgenssto produce (compressed) vectors for use as base points in the orbit algorithm over the smaller field; and a matching hash function. When later generators got added that really needed the larger field, everything exploded in confusing ways.This issue has been a thorn in my side for years, but today after several hours of debugging I finally tracked it down.