Enhance HasXXFactorGroup

[hungaborhorvath]

Please make sure that this pull request:

• is submitted to the correct branch (the stable branch is only for bugfixes)
• contains an accurate description of changes for the release notes below
• provides new tests or relies on existing ones
• correctly refers to other issues and related pull requests

Tick all what applies to this pull request

• Improves and extends functionality
• Fixes bugs that could lead to break loops

Write below the description of changes (for the release notes)

HasSolvableFactorGroup computes commutators iteratively, and checks if
they are in the normal subgroup. If DerivedSeries is already known,
then checks if its last term is in N.
HasAbelianFactorGroup and HasElementaryAbelianFactorGroup returns true if
group is already known to be abelian or elementary abelian. The latter
previously errored out if normal subgroup was the whole group, fixed.
Tests are added.

Fixes the last lingering part of #591 and applies @hulpke's suggestion from #1062.

[hungaborhorvath]

Further, if the answer is false, I set IsSolvable(G) to false, as well. However, this may be dangerous if N was given in a way that it does not contain the identity of G. Should we check about N, or should we just leave it to the user to mess up their own computations if not using the function as intended?

[hulpke]

Thank you for this. Why should N not contain the identity? It is a normal subgroup.

[hungaborhorvath]

@hulpke Yes, it should. All I am saying is that currently it is not checked whether the given N is really a normal subgroup of G. The function itself accepts improper inputs, as well (on assertion levels 0 and 1). Then it might be possible that from a solvable group G we reach the trivial group so that it has generators, therefore it will not report true, but then it could be recognized as perfect, and then not only will the function give a false result, also the IsSolvable property of the original group will be set falsely.

But this may just be my too active imagination.

[hungaborhorvath]

Is there a reason why there is no coverage report on this PR after a day?

[ChrisJefferson]

I've tried giving it a kick.

[hungaborhorvath]

@ChrisJefferson Thank you. After 10 hours, still nothing. :-( What is supposed to be the timeframe for codecov? Previously it looked to me that a couple hours and it produced its output.

[fingolfin]

Codecov normally runs immediately once Travis and AppVeyor are complete (and it is AppVeyor which takes hours).
Anyway, there were some issues wit the codecov integration which now should be resolved. Please try rebasing on master, that might help.

[codecov-io]

Current coverage is 54.48% (diff: 100%)

Merging #1066 into master will increase coverage by <.01%

@@             master      #1066   diff @@
==========================================
  Files           430        430          
  Lines        224650     224674    +24   
  Methods        3431       3431          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits         122383     122406    +23   
- Misses       102267     102268     +1   
  Partials          0          0          

Diff Coverage	File Path
•••••••••• 100%	lib/grp.gi

Powered by Codecov. Last update b7f89ca...463d548

[fingolfin]

Hmm, seems my earlier comment on this got lost? I suggested to check HasDerivedSeriesOfGroup here and if true, fallback to the previous code, which could be more efficient here. Thinking about it now, though, I think it might be faster in some cases but could also be slower in others... hmmm

[hungaborhorvath]

It does make sense in that case to check whether the last term of the derived series of G is in N, even if it might be slower in rare situations. Anyway, added to the commit, force pushed.

[fingolfin]

This looks fine to me now. Any objections to merging it?

@hulpke ?