Add LowIndexNormalSubs operation

[FriedrichRober]

• Add Operation
• Add Documentation
• Resolve Issue User Convience: Not Fp-Groups #58
• Add test for fp-group and not fp-group

Closes #57

[codecov]

Codecov Report

Merging #59 (c9de9a4) into master (8617b89) will increase coverage by 0.54%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
+ Coverage   93.92%   94.46%   +0.54%     
==========================================
  Files          12       12              
  Lines        1596     1608      +12     
==========================================
+ Hits         1499     1519      +20     
+ Misses         97       89       -8     

Files	Coverage Δ
gap/LINS.gd	100.00% <100.00%> (ø)
gap/LINS.gi	87.13% <100.00%> (+4.09%)	⬆️

[FriedrichRober]

@fingolfin, we have one problem: LowIndexNormalSubgroups in polycylic computes the normal subgroups of index n, while LowIndexNormalSubgroups in LINS computes all normal subgroups of index at most n, similar to LowIndexSubgroups

[FriedrichRober]

We probably want to have two operations, one for computing all normal subgroups with index at mostn and the other one for all normal subgroups of index equal to n. I struggle with finding two suitable names, maybe we could add the suffix OfIndex to indicate that the subgroups of index equal to n are returned.

The package polycyclic has the operations/functions LowIndexNormalSubgroups, LowIndexSubgroupClasses and MaximalSubgroupClassesByIndex that compute the subgroups of index equal to n.

Gap lib has the operations/functions LowIndexSubgroups, LowIndexSubgroupsFpGroup that compute subgroups with index at most n.

In my opinion, the behaviour of all LowIndex operations should match with one another, and we probably should stick with the convention from LowIndexSubgroups from gap lib. So for example both LowIndexSubgroups and LowIndexNormalSubgroups should return a list of (class representatives of) all (normal) subgroups with index at most n.

[fingolfin]

I generally agree that consistent behavior would be better, but that ship has sailed for now.

Perhaps over time we can carefully re-engineer the code in the GAP library and in polycyclic to offer an optional argument which allows choosing different behaviors -- e.g. choose whether to all subgroups of index $< n$ or $=n$ or dividing $n$. But that will require more work and coordination...

BTW I had a look at the polycyclic source code, and it actually computes all normal subgroups of any index dividing $n$, but then it discards any of index $< n$. So it wouldn't be too hard to change it to return more normal subgroups; or to make it accept an option to choose a behavior. But I am reluctant to change the current default because it might break existing code.

So perhaps we should just use a different name for now and implement our ideal version there.

I don't have a great suggestion, though... Perhaps LowIndexNormalSubgroupsIterator and then we return an iterator object?

[fingolfin]

We agreed to use GAP options to allow the user to customize the behavior

[FriedrichRober]

Since polycyclic uses a KeyDependentOperation, two consecutive calls of LowIndexNormalSubgroups with different options might return the wrong result and cause the user too much trouble and confusion. For this reason, we decided to rename the operation in this package to LowIndexNormalSubs and add a remark in the documentation.

[fingolfin]

This kind of test is fragile and prone to break when GAP improvements lead to different generating sets. I would recommend to hide the outputs in these cases and instead test for suitable properties (which you already do by calling Index)

[FriedrichRober]

Yes, usually I do this, I just thought that for such a small example it might actually be useful to also check the representations of the subgroups. I can remove these parts.