Call GroupByGenerators/GroupWithGenerators with a collection?

[ThomasBreuer]

@alex-konovalov had noticed a test failure,
see #1619 (comment).

This failure is a side-effect of the changes from pull request #2522.

The operations GroupByGenerators and GroupWithGenerators are documented
for a list of generators.
However, their methods accept a collection as the first argument,
and call AsList in order to create a list from it if necessary.
With the abovementioned changes, a new function MakeGroupyType was introduced
that assumes the generators to be a list, and this is where the error happens.

Thus the following works in GAP 4.9 but not in the master branch.

GroupByGenerators( Group( (1,2) ) )

I see several possibilities to fix this problem.

1. Change the method installations for GroupByGenerators and GroupWithGenerators
   such that only a list & collection or an empty list is accepted as the first argument.
   This fits to the documentation,
   the problem is that existing code may be broken that uses the undocumented feature.

2. Change the method installations for GroupByGenerators and GroupWithGenerators
   such that a non-list collection given as the first argument gets replaced by a list in the beginning.

3. Proceed like in 1. but add new methods requiring a collection as the first argument and then
   delegating to the methods for lists;
   add comments to these methods that they might become obsolete at some time.

What do others think?

[markuspf]

Is calling AsList on a group a concern?

A user might use GroupByGenerators( biggroup ), and then wonder why its slow.

[ThomasBreuer]

@markuspf First we should decide whether we want to admit a non-list collection as an argument at all.
If yes then we might think about how to turn a given non-list collection into a correct list of generators.
For the case of GroupWithGenerators, I think there is no choice,
but GroupByGenerators could of course do something more sensible.

[markuspf]

I think we shold allow collections (that are not lists), because code like above ( GroupByGenerators( Group( (1,2) ) )) looks like a fairly natural thing to do.

Now, converting just any collection to a list throws up my issue with turning things into lists that really shouldn't be; what collections would people want to use to generate groups from?

[ThomasBreuer]

@markuspf If we want to allow collections then I would add a special GroupByGenerators treatment
for collections that store one of GeneratorsOfMagmaWithInverses, GeneratorsOfMagmaWithOne, GeneratorsOfMagma --take the attribute value in question as the generators of the returned group.

(One could also think of a conjugacy class as an argument, but this is perhaps an exotic example,
and it is not clear how one wants to treat this case --by repeated closures with elements until the group contains the class?)

In all other cases (in particular for GroupWithGenerators), take the list of elements.

[olexandr-konovalov]

I never used GroupByGenerators( Group( <generators> )) - always used Group( GeneratorsOfGroup( <group> )) instead - e.g. when I want to create a new group which does not contain additional information and then test something on it. I agree that the special treatment of collections that store their generators will simplify creating such groups for the tests.

[olexandr-konovalov]

We need a fix for this problem, otherwise CTblLib tests still exhibit this kind of failures, see e.g. https://travis-ci.org/gap-system/gap-docker-pkg-tests-stable-4.10-staging/jobs/438054210

[fingolfin]

@alex-konovalov I don't see why we "need" to address this to resolve the CTblLib tests: as far as I can tell, it experiences many failures, which are not all caused by this, no?

[olexandr-konovalov]

@fingolfin I never said that fixing this will make CTblLib tests pass. Its tests have many diffs caused by other reasons - for example, there are diffs because the output may be different dependently on whether SpinSym package is loaded or not.

Still I think that this issue should have slightly higher priority - first, it's not a matter of slightly different output like in many other test failures, but a break loop happening. Second, this diff was not there before, so at least fixing it help to fight further tests deterioration.

I hope that we can fix this, and @ThomasBreuer will be able to make a new release soon, addressing remaining diffs in the tests, and then I will be able to move CTblLib from staging build (https://travis-ci.org/gap-system/gap-docker-pkg-tests-stable-4.10-staging) to the build for packages which have their tests passing (https://travis-ci.org/gap-system/gap-docker-pkg-tests-stable-4.10). I am regularly monitoring the latter build, and investigate/report when tests fail. This is not the case for the "staging" tests - if they fail, the responsibility to check whether failures are serious or harmless stays with their package authors.

Note that the "staging" build now has only 11 packages, while now 90 packages ready for GAP 4.10 release have their tests passing. I think it's a shame that among those 11 packages there are some which are loaded in GAP by default - we should be careful to not to break them, but we are not doing this efficiently. We can do better.

[olexandr-konovalov]

P.S. So, what was the conclusion about the fix or where it is needed and where - in the library by adding new methods for objects that have GeneratorsOfMagmaWithInverses or likes, or in CTblLib which calls GroupByGenerators for a group, as we can see here:

gap> s2:= CharacterTable( mx[2] );
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `Length' on 1 arguments at /Users/alexk/GITREPS/gap/lib/methsel2.g:250 called from
Length( gens ) at /Users/alexk/GITREPS/gap/lib/grp.gi:4372 called from
MakeGroupyType( FamilyObj( gens ), IsGroup and IsAttributeStoringRep and HasIsEmpty 
    and HasGeneratorsOfMagmaWithInverses and HasOne, gens, id, true 
 ) at /Users/alexk/GITREPS/gap/lib/grp.gi:4424 called from
GroupByGenerators( libtbl.AutomorphismsOfTable, () 
 ) at /Users/alexk/GITREPS/gap/pkg/ctbllib/gap4/ctadmin.tbi:815 called from
CharacterTableFromLibrary( str ) at /Users/alexk/GITREPS/gap/lib/ctbl.gi:4099 called from
<function "unknown">( <arguments> )
 called from read-eval loop at *stdin*:4
type 'quit;' to quit to outer loop
brk> libtbl.AutomorphismsOfTable;
Group([ (9,10), (7,8) ])
brk> KnownAttributesOfObject(libtbl.AutomorphismsOfTable);
[ "OneImmutable", "LargestMovedPoint", "GeneratorsOfMagmaWithInverses", 
  "MultiplicativeNeutralElement" ]

It looks that everyone in this thread was happy about adding a special treatment of objects with known generators?

[olexandr-konovalov]

There were no new developments here, and I will remove the milestone for now. The fix, if need be, can be achieved purely in CTblLib by calling GroupByGenerators(GeneratorsOfGroup(...)) and it will not help to CTblLib tests to pass, since they have other diffs at the moment anyway (see https://travis-ci.org/gap-system/gap-docker-pkg-tests-stable-4.10-staging/).