-
Notifications
You must be signed in to change notification settings - Fork 161
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 2-cohomology and module computations for arbitrary finite groups, not just solvable ones, via TwoCohomologyGeneric
#3383
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3383 +/- ##
==========================================
+ Coverage 85.17% 85.25% +0.07%
==========================================
Files 699 699
Lines 346081 346872 +791
==========================================
+ Hits 294784 295735 +951
+ Misses 51297 51137 -160
|
256316b
to
c26720d
Compare
20233f4
to
2c36816
Compare
@hulpke I noticed you’ve removed the WIP from the title and the ‘do not review’ label. Just to check, for sure: does this mean that you’d be happy for the PR to be reviewed, and hopefully merged? If so, I’d suggest removing the ‘do not merge’ label because I get the impression it might put people off from reviewing. Sorry I’ve not had chance to look at your PR further yet. |
Travis reports two manual example diffs:
|
@wilfwilson |
dc4d6f1
to
4b442f2
Compare
@wilfwilson OK, now the promised changes are in, and the holds are removed. |
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.
Very nice, thank you! Overall, this is of course a very welcome improvement!
Unfortunately I did not yet have time to look at everything (with easter and everything), but I left a few rather minor remarks. I would like to have a closer look at a few more things, I'll try to get this done this week or early next week.
Of course it would be nice to have this in GAP 4.11 -- @alex-konovalov is in charge of that.
7c5e2e3
to
aa9bfe4
Compare
else | ||
if not IsPcgs(Ggens) then | ||
elmlist:=fail; | ||
epi:=EpimorphismFromFreeGroup(G); |
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 code section is also untested with testinstall
.
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.
Still not covered.
until l=1; | ||
else | ||
#D:=Stabilizer(D,M.generators,gens,genimgs,f); | ||
hf:=SparseIntKeyVecListAndMatrix(false,Concatenation(M.generators)); |
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.
Also not covered by testinstall
.
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.
Still not covered.
|
||
# The augmentation ideal of a P-normal subgroup lies in the radical | ||
if Characteristic(F)=0 then | ||
si:=TrivialSubgroup(G); |
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.
Also not covered by testinstall
.
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.
Still not covered.
AddSet(hastail,Length(rules)); | ||
fi; | ||
elif Length(r[1])>1 then | ||
if Length(r[2])=0 then Error("generator is trivial");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.
Also not covered by testinstall
.
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.
Still not covered.
if model<>fail then | ||
q:=GQuotients(model,G)[1]; | ||
pre:=List(gens,x->PreImagesRepresentative(q,x)); | ||
ker:=KernelOfMultiplicativeGeneralMapping(q); |
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.
Also not covered by testinstall
.
Uses rewriting system since pc presentation will not always be available. Use a rewriting system based on extensions for the factor group (as it is smaller). Rename `PresentationCocycle` to `FpGroupCocycle` and offer to calculate a faithful permutation representation for it. Added test Add info from decomp. Avoid the global Gpword fct. for order issues. ENHANCE: Allow for different presentations for better rewriting Added IsomorphismFpGroupForRewriting and method for alternating groups. ENHANCE: Avoid duplicate test, build on existing permrep first. Examples reflect new code. Make immutable. Simplify presentation before trying permrep. Force natural An for presentation. REBASE: Fix wrong suggestion. ENHANCE: Two-Cohomology for arbitrary finite groups Uses rewriting system since pc presentation will not always be available. Use a rewriting system based on extensions for the factor group (as it is smaller). Rename `PresentationCocycle` to `FpGroupCocycle` and offer to calculate a faithful permutation representation for it. Added test Add info from decomp. Avoid the global Gpword fct. for order issues. ENHANCE: Allow for different presentations for better rewriting Added IsomorphismFpGroupForRewriting and method for alternating groups. ENHANCE: Avoid duplicate test, build on existing permrep first. Examples reflect new code. Make immutable. Simplify presentation before trying permrep. Force natural An for presentation. Simplify presentation before finding subgroup quotients Run Tietze in LargerAbelianQuotient with different heuristics ENHANCE: Two-Cohomology for arbitrary finite groups Uses rewriting system since pc presentation will not always be available. Use a rewriting system based on extensions for the factor group (as it is smaller). Rename `PresentationCocycle` to `FpGroupCocycle` and offer to calculate a faithful permutation representation for it. Added test Add info from decomp. Avoid the global Gpword fct. for order issues. ENHANCE: Allow for different presentations for better rewriting Added IsomorphismFpGroupForRewriting and method for alternating groups. ENHANCE: Avoid duplicate test, build on existing permrep first. Examples reflect new code. Make immutable. Simplify presentation before trying permrep. Force natural An for presentation. REBASE: Fix wrong suggestion. REBASE: Simplify presentation before finding subgroup quotients Run Tietze in LargerAbelianQuotient with different heuristics
Burnside-Brauer method for permutation groups. Generically: Factor out radical part stemming from P-core. Spin submodules spanned by augmentation ideal vectors for first submodul chain and chop, to avoid irreducibility tests in large regular module. Also flag fix for `TrivialModule`.
Uses Module isomorphism/automorphism code, modeled on Cannon/Holt autom. group paper. Also add action of compatible pairs
Includes action of compatible pairs on cohomology group and model.
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 for the added testinstall
test. There are still about 300 new lines of code which are not covered by a test in testinstall
, and the new code only has a coverage rate of ~76% as a result. I'd prefer if it was higher (not because high coverage by itself is a magic goal, but rather because it would increase my confidence in all those special cases, and also in our ability to keep this code from breaking in the future).
Anyway, I don't think we need to delay this further because of that. So let's merge it soon. (I'll wait for Travis to finish with its current truck loads of builds before merging it).
B:=MTX.ModuleAutomorphisms(M); | ||
if Size(B)>1 then | ||
for i in Set(GeneratorsOfGroup(B)) do | ||
Add(pool,DirectProductElement([One(A),i])); |
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 loop is still not covered.
|
||
if oper=fail then | ||
Ggens:=GeneratorsOfGroup(G); | ||
oper := GroupHomomorphismByImagesNC( G, Mgrp, Ggens, M.generators ); |
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.
Still not covered.
else | ||
if not IsPcgs(Ggens) then | ||
elmlist:=fail; | ||
epi:=EpimorphismFromFreeGroup(G); |
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.
Still not covered.
until l=1; | ||
else | ||
#D:=Stabilizer(D,M.generators,gens,genimgs,f); | ||
hf:=SparseIntKeyVecListAndMatrix(false,Concatenation(M.generators)); |
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.
Still not covered.
else | ||
a:=MaximalAbelianQuotient(G); | ||
si:=List(gens,x->ImagesRepresentative(a,x)); | ||
sub:=IrreducibleModules(Group(si),F,1); |
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.
Still not covered.
monreps:=fmgens{[loff+1..off]}; | ||
monreal:=mgens{[loff+1..off]}; | ||
if IsBound(m!.rewritingSystem) then | ||
k:=m!.rewritingSystem; |
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 one of only two lines in this function which is no covered. Very nice!
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, this is as a hook for future extensions (RWS for simple groups).
else x:=-i/2;fi; | ||
# word must be freely cancelled | ||
if Length(g)>0 and x=-g[Length(g)] then | ||
Unbind(g[Length(g)]); |
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 other uncovered line).
AddSet(hastail,Length(rules)); | ||
fi; | ||
elif Length(r[1])>1 then | ||
if Length(r[2])=0 then Error("generator is trivial");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.
Still not covered.
TwoCohomologyGeneric
Uses rewriting system since pc presentation will not always be available.
This is both code for the cohomology group, and for constructing the extension corresponding to a particular cocycle. Group is constructed by default as Fp group, but there is an option to have it compute a faithful permutation representation.
Also includes improved routines for
IrreducibleModules
for the non-pc case once such cohomology routines exist, and action of compatible pairs on the cohomology group.There is a test in
testextra
that, as application, constructs perfect groups anew. (This could easily go to larger orders, if desired, at the moment two quick tests are in.)