Provide TrivialGroupCons for IsFpGroup

[fingolfin]

No description provided.

[hulpke]

Looks fine, but why is there an IsFinite filter?

[codecov]

Codecov Report

Merging #2037 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2037      +/-   ##
==========================================
- Coverage   66.03%   66.02%   -0.01%     
==========================================
  Files         898      898              
  Lines      273398   273399       +1     
  Branches    12792    12792              
==========================================
- Hits       180528   180525       -3     
- Misses      90039    90042       +3     
- Partials     2831     2832       +1

Impacted Files	Coverage Δ
grp/basicfp.gi	89.1% <0%> (-0.9%)	⬇️
lib/queue.g	66.4% <0%> (-3.2%)	⬇️
src/hpc/traverse.c	77.99% <0%> (-0.78%)	⬇️
src/hpc/thread.c	45.81% <0%> (-0.2%)	⬇️
src/hpc/threadapi.c	34.74% <0%> (+0.09%)	⬆️
src/funcs.c	76.37% <0%> (+0.13%)	⬆️
src/stats.c	79.43% <0%> (+0.13%)	⬆️
hpcgap/lib/hpc/stdtasks.g	38.87% <0%> (+0.25%)	⬆️

[fingolfin]

To be honest: simply because it's there in the other TrivialGroupCons installations.

But I am not sure why it is there. I mean, in theory it affects the rank of this constructor (which is -1 times the rank of the first filter, i.e. of IsFpGroup and IsFinite); the IsFinite thus makes the method more special; in theory that would leave room for a constructor with just the filter IsFpGroup. But I can't think of any plausible reason why that would ever be useful or otherwise relevant...

Perhaps @stevelinton has some insight?

[fingolfin]

Would be nice to get feedback from @stevelinton

Other than that, I wonder if I should target this at stable-4.9 instead of master? It's not a bug fix, but OTOH it's a rather trivial change that shouldn't cause any bad regressions. I am happy either way, just wanted to mention the possibility for completeness.

[stevelinton]

For a constructor it makes sense to put every filter that the returned object is guaranteed to have in that position. With the method as is, we get:

gap> TrivialGroupCons(IsFpGroup and IsAbelian);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `TrivialGroupCons' on 1 arguments at /
.....

So having IsFinite here means that if the caller asks for their trivial group to be finite, this method will still apply, arguably IsTrivial should be there instead.

In fact, what seems to make sense is for each constructor to have a set of flags that should apply to any object produced by any method for that constructor (possibly not OtherMethods) and to silently include them in the first requirement. So all methods for TrivialGroupCons would know that they produced a trivial (and so finite, abelian, etc.) group.

[olexandr-konovalov]

I'm inclined to say yes for going into stable-4.9 branch. Perhaps after 4.9.0 will be published, I'd be more conservative, but for now yes. @fingolfin could you please switch this PR to stable-4.9 branch?

[fingolfin]

Updated:

• switched to stable-4.9
• added IsTrivial filter as suggested by @stevelinton, and also adjusted filters of several other constructors
• fixed at least one incorrect constructor filter
• documented that AbelianGroup and CyclicGroup actually allow creating infinite groups (this has been possible for several years now)
• while add it, also added (and documented) support for DihedralGroup(infinity)

Of course with these changes the PR becomes bigger. If people are concerned about this, I can also move some or all of the additional changes to a different PR.

@stevelinton With this PR, TrivialGroupCons(IsFpGroup and IsTrivial); works, but TrivialGroupCons(IsFpGroup and IsAbelian); still does not; it seems implications are not taken into account. Of course I can manually add IsAbelian, and any other implied filters, but I wonder if this should be done automatically after all? I.e. for a constructor, replace the first argument filter by its implications? An enhancement for another PR?

[stevelinton]

@fingolfin my issue crossed with your PR update. I'd suggest making this change in stable-4.9 if checks pass and trying to get this properly sorted out in 4.10.