-
Notifications
You must be signed in to change notification settings - Fork 180
Make various operations for rational matrix groups faster (fixes a performance regression from GAP 4.15.0) #6138
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
Conversation
f78c756 to
516ae73
Compare
|
|
||
| # evaluate relators | ||
| phi := IsomorphismFpGroupByGenerators(H, GeneratorsOfGroup( H )); | ||
| phi := IsomorphismFpGroupByGeneratorsNC(H, GeneratorsOfGroup( H ) : method := "fast"); |
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 is the major bottleneck of this function. Specifically, I was using the example from issue #6135, but taking the direct product with itself to make it more a bit more spicy:
v:=[ [ [ 1, 0, 0, 0, 0, 0 ], [ 0, 0, 1, 0, 0, 0 ], [ 0, 1, 0, 0, 0, 0 ], [ 0, 0, 0, 1, 0, 0 ], [ 0, 0, 0, 0, 1, 0 ], [ 0, 0, 0, 0, 0, 1 ] ], [ [ 1, 0, 0, 0, 0, 0 ], [ 0, 1, 0, 0, 0, 0 ], [ 0, 0, 0, 1, 0, 0 ], [ 0, 0, 1, 0, 0, 0 ], [ 0, 0, 0, 0, 1, 0 ], [ 0, 0, 0, 0, 0, 1 ] ], [ [ 1, 0, 0, 0, 0, 0 ], [ 0, 1, 0, 0, 0, 0 ], [ 0, 0, 1, 0, 0, 0 ], [ 0, 0, 0, 0, 1, 0 ], [ 0, 0, 0, 1, 0, 0 ], [ 0, 0, 0, 0, 0, 1 ] ], [ [ 0, 0, 0, 0, -1, 0 ], [ 0, 1, 0, 0, 0, 0 ], [ 0, 0, 1, 0, 0, 0 ], [ 0, 0, 0, 1, 0, 0 ], [ -1, 0, 0, 0, 0, 0 ], [ 0, 0, 0, 0, 0, 1 ] ], [ [ 0, 0, 0, 0, 0, 1 ], [ 0, 1, 0, 0, 0, 0 ], [ 0, 0, 1, 0, 0, 0 ], [ 0, 0, 0, 1, 0, 0 ], [ 0, 0, 0, 0, 1, 0 ], [ 1, 0, 0, 0, 0, 0 ] ], [ [ 0, 0, 0, 0, 0, -1 ], [ 0, 1, 0, 0, 0, 0 ], [ 0, 0, 1, 0, 0, 0 ], [ 0, 0, 0, 1, 0, 0 ], [ 0, 0, 0, 0, 1, 0 ], [ -1, 0, 0, 0, 0, 0 ] ] ];
W:=Group(v);
G:=DirectProduct(W,W);Then IsomorphismFpGroupByGenerators in the code I am commenting on here it takes 3.5 seconds. With the fast option it's down to ~900 milliseconds.
Alas neither is particularly great, the "old" code pre-4.15 is still faster. (Of course there are other examples where the new code is way, WAY faster, otherwise I wouldn't have put it in).
I wonder if constructive recognition would help to do this better (the group in question has very small factors: just A6 and otherwise 2-groups
516ae73 to
f018b28
Compare
This fixes a performance regression from GAP 4.15.0 and in some cases makes things actually faster than they were then.
f018b28 to
9eec33b
Compare
| ## Group([ (1,2)(3,4), (1,3)(2,4) ]), Group(()), | ||
| ## Group([ (), (1,2), (1,2)(3,4), (1,2,3), (1,2,3,4) ]) ] | ||
| ## gap> List( Irr( S4 ), chi -> StructureDescription(CentreOfCharacter(chi)) ); | ||
| ## [ "S4", "1", "C2 x C2", "1", "S4" ] |
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.
@ThomasBreuer thoughts on this change and the one below? I mean, I can also adjust the generator sets once again but I feel this is whack-a-mole...
(Of course this one here is still susceptible to a change in the order of the irreducibles. I considered using Set but "1" occurs twice. Guess I could use Collected or `Sort but that makes it even more artificial...
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.
The group is SymmetricGroup( 4 ), which uses the Irr method for symmetric groups, thus the order of the irreducibles should be stable. Showing the StructureDescription values is fine.
ThomasBreuer
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.
Looks good.
| ## Group([ (1,2)(3,4), (1,3)(2,4) ]), Group(()), | ||
| ## Group([ (), (1,2), (1,2)(3,4), (1,2,3), (1,2,3,4) ]) ] | ||
| ## gap> List( Irr( S4 ), chi -> StructureDescription(CentreOfCharacter(chi)) ); | ||
| ## [ "S4", "1", "C2 x C2", "1", "S4" ] |
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.
The group is SymmetricGroup( 4 ), which uses the Irr method for symmetric groups, thus the order of the irreducibles should be stable. Showing the StructureDescription values is fine.
…rformance regression from GAP 4.15.0) (#6138) This fixes a performance regression from GAP 4.15.0 and in some cases makes things actually faster than they were then.
|
Backported to |
This fixes a performance regression from GAP 4.15.0 and in some cases makes things actually faster than they were then.
The problem was that the preimages of the new nice monomorphism for rational matrix groups in GAP 4.15.0 was way too slow. It worked via a homomorphism to a free group, decomposed elements into words, etc. etc. -- but the inverse really is a homomorphism whose domain is a permutation group, and so we can use that to compute preimages much more quickly.
Fixes #6135