Performance additions to generic 2-cohomology and Automorphism group/Isomorphism test #4219

hulpke · 2021-01-21T16:58:21Z

Changes for Release notes:

Fix IntermediateGroup to not return fail if the index is large but subgroups exist
Add CompositionSeriesThrough

Changes that do not require release notes:

Speed up collection process for 2-cohomology
If factor permrep is large, do first try for stabilizer
when computing permdegree, avoid the bold first attempt to be too bad.
Better composition series choice in SubgroupConditionAbove -- move known parts on bottom.
Improvements in normalizers in SubgroupConditionAbove. Use NormalizerViaRadical if there is huge radical bit.
Delay bottom permrep, if bounded orbit algorithms will do the job.
Allow forcing old isomorphism test
Redo generators when searching for trick relators
Require to stabilize sets of normal subgroups (may reduce in factor)
Also added option for ContainedConjugates (does not merit mentioning in release notes)

Also Typo fix/remove outdated comment in gpfpiso

Resolves #4240.

wilfwilson

Everything seems alright to me, nice. It would be good if you could provide test cases for the brand new functionality, i.e. the 4-argument ContainedConjugates and the CompositionSeriesThrough. Otherwise I made two incredibly minor comments on your comments... 😬

lib/autsr.gi

- Speed up collection process for 2-cohomology - If factor permrep is large, do first try for stabilizer - when computing permdegree, avoid the bold first attempt to be too bad.

- Better composition series choice in `SubgroupConditionAbove` -- move known parts on bottom - Improvements in normalizers in `SubgroupConditionAbove`. Use NormalizerViaRadical if there is huge radical bit. - Delay bottom permrep, if bounded orbit algorithms will do the job. - Allow forcing old isomorphism test - Redo generators when searching for trick relators - Require to stabilize sets of normal subgroups (may reduce in factor) Also Typo fix/remove outdated comment in gpfpiso Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>

tst/teststandard/grpprmcs.tst

Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>

fingolfin · 2021-02-05T11:25:56Z

lib/autsr.gi

@@ -94,13 +94,24 @@ local C,M,p,all,gens,sub,q,hom,fp,rels,new,pre,sel,i,free,cnt;
  fi;
  p:=SmallestPrimeDivisor(Size(M));
  all:=[];
-  gens:=SmallGeneratingSet(Image(nat));
+  if newgens=true then
+    # so generators new


I don't understand this comment, "so generators new"? Maybe "compute new generators for C" or so?

fingolfin · 2021-02-05T11:28:45Z

lib/autsr.gi

    function(elm)
      if abort then
        return true; # are we bailing out since it behaves too badly?
      fi;
+      # would it contribute to avoid outside S?
+      if elm in avoid or
+        ForAny(Set(Factors(Order(elm))),e->elm^e in avoid and not elm^e in S)


Purely optional suggestion:

Suggested change

ForAny(Set(Factors(Order(elm))),e->elm^e in avoid and not elm^e in S)

ForAny(PrimeDivisors(Order(elm)),e->elm^e in avoid and not elm^e in S)

fingolfin · 2021-02-09T16:33:38Z

lib/grp.gi

+
+  SortBy(normals,x->-Size(x));
+  # check that this is a valid series
+  Assert(0,ForAll([2..Length(normals)],i->IsSubset(normals[i-1],normals[i])));


Just out of curiosity, is there a specific reason to use IsSubset instead of IsSubgroup here?

Perhaps there should also be a second assertion (perhaps run only at a higher assertion level) which verifies that normals truly consists of normal subgroups; and/or add a warning to the documentation that passing non-normal subgroups may result in wrong output?

fingolfin · 2021-02-09T16:35:42Z

lib/grp.gi

+        if not IsSubset(rev[Length(rev)],cs[i]) then
+          c:=ClosureGroup(cs[i],j);
+          if Size(c)>Size(rev[Length(rev)]) then
+            # proper down step


Why is adding a larger group a "down step"?

fingolfin · 2021-02-09T16:37:34Z

lib/grp.gi

+    # first in cs that does not contain j
+    pre:=PositionProperty(cs,x->not IsSubset(x,j));
+    # first contained in j. 
+    post:=PositionProperty(cs,x->Size(j)>=Size(x) and IsSubset(j,x));


Minor optimization possible?

Suggested change

post:=PositionProperty(cs,x->Size(j)>=Size(x) and IsSubset(j,x));

post:=PositionProperty(cs,x->(Size(j) mod Size(x) = 0) and IsSubset(j,x));

fingolfin · 2021-02-09T16:42:13Z

lib/grplatt.gd

+##  If the optional fourth argument <A>onlyone</A> is given as <A>true</A>,
+##  then only one pair (or <A>fail</A> if none exists) is returned.


Suggested change

## If the optional fourth argument <A>onlyone</A> is given as <A>true</A>,

## then only one pair (or <A>fail</A> if none exists) is returned.

## If the optional fourth argument <A>onlyone</A> is given as <K>true</K>,

## then only one pair (or <K>fail</K> if none exists) is returned.

fingolfin · 2021-02-09T16:43:11Z

lib/grplatt.gi

+DoContainedConjugates:=function(arg)
+local G,A,B,onlyone,l,N,dc,gens,i;
+  G:=arg[1];
+  A:=arg[2];
+  B:=arg[3];
+  if Length(arg)>3 then onlyone:=arg[4]; else onlyone:=false;fi;


Could also do this:

Suggested change

DoContainedConjugates:=function(arg)

local G,A,B,onlyone,l,N,dc,gens,i;

G:=arg[1];

A:=arg[2];

B:=arg[3];

if Length(arg)>3 then onlyone:=arg[4]; else onlyone:=false;fi;

DoContainedConjugates:=function(G,A,B,onlyone...)

local l,N,dc,gens,i;

if Length(onlyone)>0 then onlyone:=onlyone[1]; else onlyone:=false;fi;

Or this

Suggested change

DoContainedConjugates:=function(arg)

local G,A,B,onlyone,l,N,dc,gens,i;

G:=arg[1];

A:=arg[2];

B:=arg[3];

if Length(arg)>3 then onlyone:=arg[4]; else onlyone:=false;fi;

DoContainedConjugates:=function(G,A,B,onlyone)

local l,N,dc,gens,i;

and then change one InstallMethod below to use the following function instead: {G,A,B}->DoContainedConjugates(G,A,B,false)

fingolfin · 2021-02-09T16:49:23Z

Argh, terrible timing :-)

I just was about to submit my review when you merged :-(.

Sorry, i was extremely busy the past couple weeks and missed this one.

hulpke · 2021-02-09T16:50:27Z

@fingolfin
Sorry -- if you know how to unmerge, I will go through your suggestions.

fingolfin · 2021-02-09T16:52:10Z

No, you can't "unmerge", and I think it's fine. I am sorry for being late to the game; and there was nothing terribly important. If you think any of my comments deserve an adjustment, you can just open a new PR. Or ignore them

hulpke added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes kind: performance labels Jan 21, 2021

hulpke force-pushed the additions branch 2 times, most recently from af07733 to f0d7eda Compare January 22, 2021 19:53

hulpke force-pushed the additions branch from 9c51ed3 to 5ceb087 Compare January 29, 2021 17:42

hulpke mentioned this pull request Feb 4, 2021

Inconsistent constructions with SmallGroups #4043

Closed

hulpke force-pushed the additions branch from 5ceb087 to c57eac0 Compare February 5, 2021 19:59

hulpke mentioned this pull request Feb 5, 2021

conjugate a subgroup into a given one #4240

Closed

hulpke force-pushed the additions branch from 5b644d3 to 177da5c Compare February 5, 2021 21:15

hulpke added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Feb 7, 2021

hulpke force-pushed the additions branch from ce446ff to 493d8d1 Compare February 7, 2021 19:39

wilfwilson added the kind: bug Issues describing general bugs, and PRs fixing them label Feb 8, 2021

wilfwilson approved these changes Feb 8, 2021

View reviewed changes

lib/autsr.gi Outdated Show resolved Hide resolved

lib/autsr.gi Outdated Show resolved Hide resolved

hulpke and others added 6 commits February 8, 2021 12:48

ENHANCE: Added CompositionSeriesThrough

0d5f49e

ENHANCE: Performance improvements generic 2-cohomology

b8bf767

- Speed up collection process for 2-cohomology - If factor permrep is large, do first try for stabilizer - when computing permdegree, avoid the bold first attempt to be too bad.

ENHANCE: `onlyone'' option for ContainedConjugates`

1474417

FIX: IntermediateGroup will not exit quietly if index is large

1e70559

TEST: Added test for CompositionSeriesThrough

2dbbe7f

hulpke force-pushed the additions branch from 520cb9b to 2dbbe7f Compare February 8, 2021 21:06

wilfwilson reviewed Feb 9, 2021

View reviewed changes

tst/teststandard/grpprmcs.tst Show resolved Hide resolved

Update tst/teststandard/grpprmcs.tst

3506a28

Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>

hulpke merged commit 6532f3b into gap-system:master Feb 9, 2021

fingolfin reviewed Feb 9, 2021

View reviewed changes

hulpke mentioned this pull request Feb 9, 2021

problems with IntermediateGroup and IntermediateSubgroups #4246

Closed

ThomasBreuer self-assigned this Feb 16, 2021

ThomasBreuer added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Feb 16, 2021

ThomasBreuer removed their assignment Feb 16, 2021

olexandr-konovalov mentioned this pull request Feb 19, 2021

Simplify the creation of release notes #4257

Closed

olexandr-konovalov mentioned this pull request Mar 23, 2021

Python script to get an overview of changes with PyGithub #4277

Merged

fingolfin added topic: performance bugs or enhancements related to performance (improvements or regressions) and removed kind: performance labels Apr 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance additions to generic 2-cohomology and Automorphism group/Isomorphism test #4219

Performance additions to generic 2-cohomology and Automorphism group/Isomorphism test #4219

hulpke commented Jan 21, 2021 •

edited by fingolfin

Loading

wilfwilson left a comment

fingolfin Feb 5, 2021

fingolfin Feb 5, 2021

fingolfin Feb 9, 2021

fingolfin Feb 9, 2021

fingolfin Feb 9, 2021

fingolfin Feb 9, 2021

fingolfin Feb 9, 2021

fingolfin Feb 9, 2021

fingolfin Feb 9, 2021

fingolfin commented Feb 9, 2021

hulpke commented Feb 9, 2021

fingolfin commented Feb 9, 2021

	ForAny(Set(Factors(Order(elm))),e->elm^e in avoid and not elm^e in S)
	ForAny(PrimeDivisors(Order(elm)),e->elm^e in avoid and not elm^e in S)

	post:=PositionProperty(cs,x->Size(j)>=Size(x) and IsSubset(j,x));
	post:=PositionProperty(cs,x->(Size(j) mod Size(x) = 0) and IsSubset(j,x));

		## If the optional fourth argument <A>onlyone</A> is given as <A>true</A>,
		## then only one pair (or <A>fail</A> if none exists) is returned.

Performance additions to generic 2-cohomology and Automorphism group/Isomorphism test #4219

Performance additions to generic 2-cohomology and Automorphism group/Isomorphism test #4219

Conversation

hulpke commented Jan 21, 2021 • edited by fingolfin Loading

wilfwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Feb 9, 2021

hulpke commented Feb 9, 2021

fingolfin commented Feb 9, 2021

hulpke commented Jan 21, 2021 •

edited by fingolfin

Loading