Not all finitely generated magmas are groups

[fingolfin]

... hence we should not set IsFinitelyGeneratedGroup for them. Instead, only set IsFinitelyGeneratedGroup if the family indicates that its elements are associative.

Note that this all is only done to avoid a call to the default immediate method for IsFinitelyGeneratedGroup. I am not really sure whether this is worth the hassle... Perhaps it is when one creates thousands and thousands of groups in a tight loop?

Fixes #2252

[ThomasBreuer]

The point is whether the result knows that it is associative.
The proposed code checks one possibility why this can be the case.
Wouldn't it be simpler and more general to check the result whether IsAssociative is set in it, and then to set IsFinitelyGeneratedGroup?
(I had proposed this already in a comment to #2252.)

[fingolfin]

Sure, that would be simpler, but it would also be redundant, as by that time, the immediate method would have been triggered anyway. So, this would be equivalent to simply not setting IsFinitelyGeneratedGroup in the initial filter list. I'd be happy with that, too. Not sure if @hulpke is, though?

[ThomasBreuer]

Just one remark about immediate methods:
According to the documentation, one cannot rely on the fact that this feature is switched on at all.

[fingolfin]

Of course that's what it says -- and I consider this claim of the manual to be rather unrealistic, because I am pretty sure nobody ever tests GAP with IGNORE_IMMEDIATE_METHODS set to true.

But anyway, I understand this remark in the manual as saying that correctness of code should not rely on immediate methods -- which we wouldn't be here, would we? But not running immediate methods certainly can result in changes in the method selection, and in possibly selecting worse methods. This is what would happen for all kinds of objects and properties already now, e.g. IsFinite, IsTrivial etc. would not be set once e.g. the size of a group is known. Why would we worry more about IsFinitelyGeneratedGroup than any of these?

[fingolfin]

Anyway, I'll prepare an alternative PR introducing IsFinitelyGeneratedMagma.

[codecov]

Codecov Report

Merging #2276 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2276      +/-   ##
==========================================
+ Coverage   70.85%   70.85%   +<.01%     
==========================================
  Files         480      480              
  Lines      253195   253198       +3     
==========================================
+ Hits       179413   179416       +3     
  Misses      73782    73782

Impacted Files	Coverage Δ
lib/magma.gi	54.59% <100%> (+0.17%)	⬆️
hpcgap/lib/hpc/queue.g	66.4% <0%> (-3.2%)	⬇️
src/hpc/traverse.c	94.97% <0%> (-0.48%)	⬇️
src/c_oper1.c	91.47% <0%> (+0.16%)	⬆️
hpcgap/lib/hpc/stdtasks.g	38.87% <0%> (+0.25%)	⬆️

[james-d-mitchell]

I don't think this should be merged, rather the simpler approach of letting the immediate method for IsFinitelyGeneratedGroup fire, and avoid the extra complication that this approach.

[fingolfin]

@james-d-mitchell I'd be fine with this, I still plan to prepare that alternate PR using IsFinitelyGeneratedMagma resp. IsFinitelyGenerated, I can make a third one which does as @ThomasBreuer suggests, and of course it'd be trivial to go from that to the radical variant you propose.

In any case, it would be very good if @hulpke could chime in, as this code is by him.

Also, to underline my conjecture that nobody tests with IGNORE_IMMEDIATE_METHODS set to true, I run the tests, and looked at the test failures, and immediately found some which show that already code violates the idea that code should not rely on immediate methods firing . Here is one example (which at least doesn't give a mathematically wrong result, it just runs into an unnecessary error):

gap> IGNORE_IMMEDIATE_METHODS;
false
gap>
gap> F := FreeGroup("r", "s");; r := F.1;; s := F.2;;
gap> G := F/[s^2, s*r*s*r];;
gap> H := DerivedSubgroup(G);
Group(<fp, no generators known>)
gap> IsSolvableGroup(G);
true
gap>
gap> IGNORE_IMMEDIATE_METHODS:=true;;
gap> F := FreeGroup("r", "s");; r := F.1;; s := F.2;;
gap> G := F/[s^2, s*r*s*r];;
gap> H := DerivedSubgroup(G);
Group(<fp, no generators known>)
gap> IsSolvableGroup(G);
Error, Derived subgroup has infinite index, cannot represent
...

There are lots of similar examples, this was just the first.

Of course an existing bug does not justify adding another one; but a rule that is already violated in many places perhaps should be re-evaluated?