call GroupByGenerators, GroupWithGenerators with domain

[ThomasBreuer]

change the default methods for GroupByGenerators
and GroupWithGenerators such that the problem gets fixed
that had been introduced with pull request #2522
and discussed in issue #2703

[fingolfin]

Hmm. If we do this for GroupByGenerators, shouldn't we also add similar methods to all other *ByGenerators* functions? But there are dozens of these. So I am rather inclined to go the other direction, and say we shouldn't do this at all...

I found these *ByGenerator operations:

AdditiveGroupByGenerators
AdditiveMagmaByGenerators
AdditiveMagmaWithInversesByGenerators
AdditiveMagmaWithZeroByGenerators
AlgebraByGenerators
AlgebraWithOneByGenerators
BiAlgebraModuleByGenerators
DefaultFieldByGenerators
DefaultRingByGenerators
DivisionRingByGenerators
DomainByGenerators
FLMLORByGenerators
FLMLORWithOneByGenerators
FieldByGenerators
FieldOverItselfByGenerators
GroupByGenerators
IdealByGenerators
InducedPcgsByGenerators
InducedPcgsByGeneratorsNC
InverseMonoidByGenerators
InverseSemigroupByGenerators
IsomorphismFpGroupByGenerators
IsomorphismFpGroupByGeneratorsNC
LeftAlgebraModuleByGenerators
LeftIdealByGenerators
LeftMagmaIdealByGenerators
LeftModuleByGenerators
MagmaByGenerators
MagmaIdealByGenerators
MagmaWithInversesByGenerators
MagmaWithOneByGenerators
MonoidByGenerators
NaturalHomomorphismByGenerators
NearAdditiveGroupByGenerators
NearAdditiveMagmaByGenerators
NearAdditiveMagmaWithInversesByGenerators
NearAdditiveMagmaWithZeroByGenerators
OrbitStabilizerListByGenerators
RepresentativeTomByGenerators
RepresentativeTomByGeneratorsNC
RightAlgebraModuleByGenerators
RightIdealByGenerators
RightMagmaIdealByGenerators
RingByGenerators
RingWithOneByGenerators
SemigroupByGenerators
SemigroupIdealByGenerators
SemiringByGenerators
SemiringWithOneAndZeroByGenerators
SemiringWithOneByGenerators
SemiringWithZeroByGenerators
TwoSidedIdealByGenerators

[ThomasBreuer]

Perhaps the discussion of issue #2703 was not explicit enough:
The point is just to fix a side-effect (which I would call a bug) of pull request #2522,
which breaks functionality in GAP 4.10 that was available in earlier versions.

[fingolfin]

But the old code never did something like this. It just took the set of elements of the given domain / collection as generators.

[fingolfin]

This completely changes formatting, but (as far as I could tell) really only inserts a single, line, gens:= AsList( gens );, and then replaces one AsList(gens) later by gens.

I'd really prefer if this changes was kept to the minimum, for ease of reviewing.

[ThomasBreuer]

Note that gens is used by MakeGroupyType.
This is where the error happens in the old version.

[fingolfin]

Same as above.

[fingolfin]

I am not even sure what has changed in this function. Was anything changed, other than rearranging the code and reformatting it?

[olexandr-konovalov]

I am thinking whether it will be easier and more consistent to adapt CTblLib to call GroupByGenerators(GeneratorsOfGroup(...)).

First of all, as we can see from the list collected by @fingolfin, doing this consistently would require changing more methods.

Second, while it seemed a good idea to me initially, it's not any more. The meaning of the names GroupByGenerators and GroupByGenerators seems intuitively clear to me as returning the structure whose generators are given as its argument. Following this logic, if it's called with a domain, then its list of generators is the list of all elements of the domain. Doing some reduction "under the hood" (and not in all "StructureByGenerators" cases) may be more confusing IMHO.

[ThomasBreuer]

@alex-konovalov I disagree completely.
Pull request #2522 has broken functionality of GAP.
According to your comments about issue #2703, you were aware of this fact,
and still you have released GAP 4.10.
When GAP users upgrade to version 4.10 (which fortunately happens slowly)
then they will run into errors, due to the broken functionality.
I think that the bug simply has to be fixed,
independent of considerations how a better world could look like.

[olexandr-konovalov]

@ThomasBreuer resolving this problem was classified as highly desirable, but not critical in #2804, therefore it was not considered as blocking the release.

Moreover, from time to time we are making changes that break backwards compatibility, and we are permitting this to happen in major releases. Sometimes this happens due to introducing stricter checks of arguments, like e.g. in #2107, #2756, so breaking things results in a more robust and efficient code.

Consider the following example that was possible before in GAP 4.9 but will not happen in GAP 4.10:

gap> S:=SymmetricGroup(8);
Sym( [ 1 .. 8 ] )
gap> GeneratorsOfGroup(S);
[ (1,2,3,4,5,6,7,8), (1,2) ]
gap> T:=GroupByGenerators(S);
<permutation group with 40320 generators>
gap> U:=Group([(1,2,3,4,5,6,7,8), (1,2) ]);
Group([ (1,2,3,4,5,6,7,8), (1,2) ])
gap> ConjugacyClasses(U);;time;
1774
gap> ConjugacyClasses(T);;time;
23195

Note that the new behaviour of GroupByGenerators and GroupWithGenerators perfectly matches their documentation, which says that they return the group G generated by the list gens. Thus, the change explicitly forbids using it in a way that was undocumented but previously (accidentally?) allowed. Therefore, I believe that the right way to resolve this would be to fix the code which used to work before because of using this undocumented feature, and not to restore that undocumented feature.

[olexandr-konovalov]

I explained in the recent comment my arguments against accepting this PR. In addition, it changes the behaviour of GroupByGenerators and GroupWithGenerators by allowing them to accept domains as arguments, but does not document that, and that makes this pull request incomplete.

[ThomasBreuer]

@alex-konovalov I disagree completely.

• When you refer to the documentation then you just cite the text;
  however, both the operation declarations and the method installations state
  that the first argument is expected to be a list or collection.
• I have deliberately not proposed a change of the documentation,
  because I think that the discussion is not yet finished
  concerning what we want in the long run.
• You give the impression as if the changed behavior in GAP 4.10 was intended.
  This is of course wrong, as one can see from the code and the discussion.

[olexandr-konovalov]

@ThomasBreuer I agree that both of us may disagree. I for example disagree with calling this a bug in GAP - IMHO it's a side effect of another change, which happen to reveal some inefficiencies in some other code.

Nevertheless, I agree to go for #3095 which has lesser scope than this PR, just restoring the previous behaviour, and still keeping it undocumented. I hope that this will fix several diffs in CTblLib package for now, but not all of them - can I just remind you since you may not have seen updates in #1619 that a new release of CTblLib with cleanly passing tests (as well as a new release of AtlasRep) will be most welcomed!

I intend to add a warning (for master branch, to appear in GAP 4.11) to be displayed when GroupWithGenerators is called on something which is not a list - if that will be accepted, it will cause warnings for CTblLib tests in their current state.