Fix regression in group constructors for matrix groups

[fingolfin]

This fixes the regression reported by @alex-konovalov

(The fact that the manual examples uncovered this motivated me to submit issue #1076).

I still find these constructors a bit weird and arbitrary. Section 50.1 says that you can spcecify a ground field for matrix groups. Fine, but there in realit only methods for cyclic groups and quaternion groups, but not for all the others (abelian, extraspecial, elementary abelian, free abelian, Mathieu group). Also you can specify a dimension (and even if you could, then in general that would of course not define a unique representation).

Anyway, those are not excuses for breaking something :-).

[codecov-io]

Current coverage is 54.47% (diff: 100%)

Diff Coverage	File Path
•••••••••• 100%	grp/basic.gd

No coverage report found for master at 53e55ac.

Powered by Codecov. Last update 53e55ac...2f99cf4

[fingolfin]

The codecov/patch status is marked as failing because this PR adds code lines, and only 25% of them have coverage... No surprise, as the others are not yet used. Now, I see three options to deal with that:

1. Ignore this failure and merge the PR anyway.
2. Add tests which trigger the code paths (and naturally produce errors; but luckily our .tst fails can handle that).
3. Remove the unused constructor subcases again
4. Remove all those constructor subcases (or rather: don't add them back, as the first commit in this PR does), and remove the code for the IsMatrixGroup constructors for QuaternionGroup and CyclicGroup.

Personally, I'd favor option 4, as I think it's silly to have these constructors in the first place. However, that might break some people's code, so 3 is more realistic. What do others think?

@hulpke @frankluebeck @stevelinton You've been around a long time, perhaps you have some insights and / or oppinions on this?

For reference, I just checked, and the IsMatrixGroup constructor for QuaternionGroup was added 2011-04-05 by Jack Smith. The one for CyclicGroup was already present in the initial import of basicmat.gi, i.e. in a commit from 1997-01-15 by Frank Celler.

[markuspf]

The patch itself of course looks fine.

On the subject of constructor cases, I don't have any strong opinions.

[hulpke]

I suspect the cyclic/quaternion matrix constructors wre to have some easy test cases. I would not have bothered to implement them, but I'm reluctant to remove functionality that is in itself not causing problems.

I'd be happy to remove the unused subcases -- for me actual code behavior overrides never implemented manual promises. So my choice would be 3.

[fingolfin]

@hulpke Yeah, I thought about it some more since writing my last comment, and came to the conclusion that it's always bad to break existing code, so I now also favor option 3.

Though I still wonder whether to remove it from the manual, and just keep the code around, with a brief comment explaining that they are just there for backwards compatibility?