Fix bug in IrrConlon leading to unexpected errors

[fingolfin]

Reported by Benjamin Sambale

Please use the following template to submit a pull request, filling
in at least the "Text for release notes" and/or "Further details".
Thank You!

Description

Text for release notes

If this pull request should be mentioned in the release notes,
please provide a short description matching the style of the GAP
Changes manual.

Further details

If necessary, please provide further details here.

Checklist for pull request reviewers

• proper formatting

If your code contains kernel C code, run clang-format on it; the
simplest way is to use git clang-format, e.g. like this (don't
forget to commit the resulting changes):

git clang-format $(git merge-base HEAD master)

• usage of relevant labels

  1. either release notes: not needed or release notes: to be added
  2. at least one of the labels bug or enhancement or new feature
  3. for changes meant to be backported to stable-4.X add the backport-to-4.X label
  4. consider adding any of the labels build system, documentation, kernel, library, tests

• runnable tests

• lines changed in commits are sufficiently covered by the tests

• adequate pull request title

• well formulated text for release notes

• relevant documentation updates

• sensible comments in the code

[ChrisJefferson]

Out of interest, what is the change of behaviour here? Perhaps put a little more in the commit message. I'm assuming a bug was fixed, which is good :)

[fingolfin]

The bug was that certain calls to IrrConlon (e.g. IrrConlon(SmallGroup(8,2));, see test file) lead to unexpected errors, which is now fixed.

That's pretty much the title of the PR, the only thing missing is the concrete example. Do you mean you'd like me to add something like this to the body of the message:

For example, calling IrrConlon(SmallGroup(8,2)); used to run into an error, and now succeeds.

[ChrisJefferson]

I didn't understand why SubgroupNC broke things, when changing to SetParent did, as I thought that setting parent was basically all SubgroupNC did.

[fingolfin]

Ahh! Well, SubgroupNC expects a parent plus a list of generators of the subgroup. Now we give it the full subgroup as "generators". Which is bad by itself, but it also means that Length( gens ) fails. This used to work in GAP 4.9, by the way, but probably never was a good idea, so overall I think it's a good idea to make this change anyway. It also ensures that we retain other known properties and attributes of the subgroup, such as its size.

[fingolfin]

Note SubgroupNC does not just set the parent; it creates a completely new group object.

[ChrisJefferson]

Ah, now I understand, this isn't some kind of subtle maths bug, just giving a function the completely wrong things :)

[fingolfin]

Good :-). I'll update the commit message nevertheless.

[ChrisJefferson]

I've approved. You could if you want add to the commit message something like As SubgroupNC takes a list of generators, not a group, just to guide some future reader, but it's not required.

[fingolfin]

Updated. BTW, in practice the SetParent call has no effect, because t is created as a stabilizer in G and thus normally already has G as parent (I verified this by inserting some assertions). But I think it doesn't hurt to set the parent explicitly either.

[ThomasBreuer]

Thanks for this fix.
(It is not the first instance of this problem.)
Concerning the changes, I would perhaps suppress showing the test output; changes in GAP's output format (which happened in the past) will make updates necessary, and the output itself is unimportant here.
As you wrote, StabilizerOfExternalSet sets already a parent; I would omit the SetParent call then, note that it could be misleading if StabilizerOfExternalSet would set a different parent object.

[fingolfin]

@ThomasBreuer both fine by me; perhaps I should replace the SetParent call by an assertion which verifies the parent...

And I'll grep through the code for calls to SubgroupNC to see if I can spot some other similar issues (I don't promise to perform a thorough search, just a quick look if I see "obvious" related cases).

[fingolfin]

I saw no obvious similar issues in SubgroupNC calls (but again, I make no claims there are none). Updated the PR again.

[ThomasBreuer]

@fingolfin Thanks for the changes.

The current documentation of Subgroup and SubgroupNC does actually not specify that the second argument must be a list. It says

‣ Subgroup( G, gens ) ───────────────────────────────────────── function
‣ SubgroupNC( G, gens ) ─────────────────────────────────────── function
‣ Subgroup( G ) ───────────────────────────────────────────── function

creates the subgroup U of G generated by gens. The Parent (31.7-1) value of
U will be G. The NC version does not check, whether the elements in gens
actually lie in G.
[...]

A better description would be something like the following.

returns the subgroup U of G generated by the elements in the list gens.
The Parent (31.7-1) value of U will be G.
The NC version does not check, whether the elements in gens actually lie in G.

Should I create a new pull request for such a change of the documentation?
And should the documentation of other Subsomething and SomethingByGenerators operations also be made precise in the sense that the generators must be given as a list, not as an arbitrary collection, even if one does not (yet) get an error if a domain is given instead of a list?

[coveralls]

Coverage increased (+0.009%) to 84.686% when pulling 783e572 on fingolfin:mh/IrrConlon into 46dcecd on gap-system:master.

[fingolfin]

@ThomasBreuer that sounds useful. Of course we could also decide that we want to allow collections, and change the code to allow for that (again). But then in Group, Magma, MagmaWithOne, etc., we explicitly say gens should be a list, so it seems sensible to require the same for Subgroup etc. as well.

[fingolfin]

Backported to stable-4.11 in commit 4de3c19