Another method for minimum generating set for finite groups

[pranav-joshi-iitgn]

I have added a function MinimalGeneratingSetUsingChiefSeries that computes a minimum generating set for finite groups using the algorithm derived from the research paper by Dhara Thakkar and Andrea Lucchini. We (me and Ruchit Jagodara) tried doing the same thing in SageMath first. The pull request is yet to be merged.

The algorithm works as shown in this flowchart :

[pranav-joshi-iitgn]

@fingolfin could you please review this PR ? This is the same algorithm that you reviewed in the PR to SageMath.

[ChrisJefferson]

My initial comment is maybe add some tests? I'm guessing you understand the types of groups you need to hit the various cases in the algorithm, and make sure it is fully tested?

[fingolfin]

Thank you for your contribution, much appreciated!

Unfortunately the code has a lot of issues (I pointed out a couple of them), and it is difficult to read and/or match to the paper. Adding some more comments would make it easier to review and thus increase the chance of it being merged.

But sadly as it is right now, the code quality is not at a level that would make it acceptable to merge this.

[ChrisJefferson]

I have some small comments, but I'd say the BIG missing thing is tests, lots of tests!

As a suggestion, you could base you tests on something like this:

checkMinimalGeneratingSet := function(G,x)
local genset;
  genset := MinimalGeneratingSet(G);
  if Group(genset, Identity(G)) <> G then Print(genset, " does not generate ", G); fi;
  if Length(genset) <> x then Print(genset, " of ", G, " is size ", Length(genset), " instead of ", x); fi;
end;

As (I think!) this is basically what we want to check -- we make the write group, and the set has the right size.

Then, you can just call checkMinimalGeneratingSet with lots of pairs of group, genset size (which you can hopefully get from another source).

In case you wonder, that Group(genset, Identity(G)) is to cover the case where genset is empty, in which case you need to explictly give the identity of the group, so Group know what type of group it is making.

[pranav-joshi-iitgn]

I have some small comments, but I'd say the BIG missing thing is tests, lots of tests!

As a suggestion, you could base you tests on something like this:

checkMinimalGeneratingSet := function(G,x)
local genset;
  genset := MinimalGeneratingSet(G);
  if Group(genset, Identity(G)) <> G then Print(genset, " does not generate ", G); fi;
  if Length(genset) <> x then Print(genset, " of ", G, " is size ", Length(genset), " instead of ", x); fi;
end;

As (I think!) this is basically what we want to check -- we make the write group, and the set has the right size.

Then, you can just call checkMinimalGeneratingSet with lots of pairs of group, genset size (which you can hopefully get from another source).

In case you wonder, that Group(genset, Identity(G)) is to cover the case where genset is empty, in which case you need to explictly give the identity of the group, so Group know what type of group it is making.

I have added a test (opers/MinimalGeneratingSetUsingChiefSeries.tst) that checks equality between the size of outputs of MinimalGeneratingSet and MinimalGeneratingSetUsingChiefSeries, and if the latter generates the group.

I am eager to know the small comments as well.

[ChrisJefferson]

I don't think this test ever executes the code from line 76 onwards

[pranav-joshi-iitgn]

I don't think this test ever executes the code from line 76 onwards

Yes, the first time that block runs is for SmallGroup(120,34), which I have added now.

[pranav-joshi-iitgn]

Any other changes that I should make ?

[ChrisJefferson]

There is no documentation at the moment.

I think in this case (but I might be wrong, feel free to comment!) this probably shouldn't be used by default for MinimalGeneratingSet, as there are cases where it is much faster and cases where it is much slower? If that's true, then it would make more sense to add it as another option, and try to give a hint when this method should be used.

[pranav-joshi-iitgn]

I added a paragraph in documentation for MinimalGeneratingSet which mentions it as another option.