Show a warning when GroupWithGenerators called on a domain

[olexandr-konovalov]

In GAP <= 4.9, the following undocumented use of GroupWithGenerators
was possible:

gap> GroupWithGenerators( Group( (1,2) ) );
Group([ (), (1,2) ])

This does not work in GAP 4.10.0, and #3095 restored this never documented
behavior to avoid regressions in code that used to work previously.

This PR adds a warning when such use of GroupWithGenerators is detected.

[markuspf]

But... why?

[olexandr-konovalov]

@markuspf what do you mean by "why?"? The commit message explains the reasons, and you may also see the discussion in #3095 and related PRs/issues.

InfoPerformance sounds like an optimal info level for such messages, and it's default value is 1 when GAP starts, but is set to 0 in regression tests, so this message will not show up there.

[ChrisJefferson]

Are you sure this mesage is true for all domains? I think could make a domain where taking its generators and making a group from them would produce a different answer (although in such a case the user is doing something very strange...)

[olexandr-konovalov]

@ChrisJefferson Do you think it should only check for IsGroup instead of IsDomain? Indeed, the way how the domain is generated by its generators may be different.

[ChrisJefferson]

Group feels like an easy case to catch -- as previously discussed we could even then just special case groups, but then we are going in a loop :) I don't know enough about how domains are defined to know if there is a good way of catching and warning about exactly the right things.

[codecov]

Codecov Report

Merging #3099 into master will decrease coverage by 8.39%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #3099      +/-   ##
==========================================
- Coverage   83.56%   75.16%    -8.4%     
==========================================
  Files         686      680       -6     
  Lines      336759   328485    -8274     
==========================================
- Hits       281401   246900   -34501     
- Misses      55358    81585   +26227

Impacted Files	Coverage Δ
lib/grp.gi	79.1% <33.33%> (-9.87%)	⬇️
lib/teachm2.g	15.38% <0%> (-84.62%)	⬇️
src/modules.h	16.66% <0%> (-83.34%)	⬇️
lib/proto.gi	1.03% <0%> (-82.48%)	⬇️
lib/ctbllatt.gi	0.81% <0%> (-79.99%)	⬇️
lib/ctblauto.gi	5.98% <0%> (-78.04%)	⬇️
lib/algliess.gi	0.99% <0%> (-74.54%)	⬇️
lib/attr.gi	27.27% <0%> (-72.73%)	⬇️
lib/ctblpope.gi	1.77% <0%> (-72.39%)	⬇️
lib/contfrac.gi	30% <0%> (-67.15%)	⬇️
... and 287 more

[olexandr-konovalov]

Indeed,GeneratorsOfDomain for a domain which is not a group may be generators w.r.t. different operation(s), so for that case it might have sense to use GroupWithGenerators on the whole domain. I've pushed the new version which uses IsGroup instead of IsDomain.

[fingolfin]

But isn't the actual problem that using a large number of generators is usually inefficient? Then, shouldn't the check instead be on the size of gens? And then, shouldn't it apply equally to lists as to domains or any other kind of collection?

[olexandr-konovalov]

But how then would you know when the number is large? I assume that the number of generators is usually smaller than the number of elements in the group (unless it is already given using a very inefficient list of generators).

[fingolfin]

Sure, any number we'd pick as "large" would be arbitrary. Just like it is arbitrary to allow Group(AsList(G)) as generators, but not Group(G). So my personal preference would be to do neither, i.e., exactly as it is right now...

[olexandr-konovalov]

Can't think of any scenario when calling Group(Group(...)) is more efficient than Group(GeneratorsOfGroup(...)) - that's what I am suggesting to catch. Not other inefficiencies, as that would be more ambiguous...