Fix AsSemigroup to reject non-associative inputs & declare IsCommutative/IsAssociative for IsCollection

[wilfwilson]

See #4030.

It might not be fast, but I think it is necessary.

I thought about adding a similar check to SemigroupByGenerators, but that (via the documentation for Semigroup) is explicitly documented as not checking for associativity.

Fixes #4030.

Text for release notes

• Check for associativity in AsSemigroup, and correctly return fail for non-associative collections, such as non-associative magmas.
• Declare IsCommutative and IsAssociative more generally for IsCollection, rather than just for IsMagma.

(End of text for release notes)

[ThomasBreuer]

Yes, the associativity test is necessary.
The problem is actually worse:

gap> T:= [ [ 1, 2, 6, 5, 4, 3 ], [ 2, 1, 4, 2, 2, 5 ], [ 6, 5, 1, 6, 6, 4 ], 
>          [ 5, 6, 3, 4, 1, 2 ], [ 4, 3, 2, 1, 5, 6 ], [ 3, 4, 5, 3, 3, 1 ] ];;
gap> M:= MagmaByMultiplicationTable( T );
<magma with 6 generators>
gap> IsAssociative( M );
false
gap> AsSemigroup( M );
<semigroup of size 6, with 3 generators>
gap> GroupByGenerators( M ); # AsGroup( M ) returns fail, which is o.k.
<group with 6 generators>
gap> MonoidByGenerators( M ); # AsMonoid(M) runs into an error
<monoid with 6 generators>
gap> Group( Elements( M ) );
<group with 6 generators>

[wilfwilson]

Thanks for the further information @ThomasBreuer. I'll investigate how that behaviour corresponds to the documentation, and try to add fixes to this PR as appropriate.

[ThomasBreuer]

@wilfwilson I have been thinking about installing a default IsAssociative method for IsCollection, and calling this in the various places where the check is needed. It is not completely clear to me where these places are, apparently several operations are affected.

[wilfwilson]

I've now updated this along the lines you suggested @fingolfin, and removed the "do not merge" label. Let's see what the tests and code coverage say.

[wilfwilson]

@ThomasBreuer here's how things lie with this PR:

gap> T:= [ [ 1, 2, 6, 5, 4, 3 ], [ 2, 1, 4, 2, 2, 5 ], [ 6, 5, 1, 6, 6, 4 ],
>          [ 5, 6, 3, 4, 1, 2 ], [ 4, 3, 2, 1, 5, 6 ], [ 3, 4, 5, 3, 3, 1 ] ];;
gap> M:= MagmaByMultiplicationTable( T );
<magma with 6 generators>
gap> IsAssociative( M );
false
gap> AsSemigroup( M );
fail
gap> AsSubsemigroup( M, M );
fail
gap> GroupByGenerators( M );
<group with 6 generators>
gap> Group( Elements( M ) );
<group with 6 generators>
gap> MonoidByGenerators( M );
<monoid with 6 generators>
gap> SemigroupByGenerators( M );
<semigroup with 6 generators>

Shall we open a separate issue about this?

[wilfwilson]

This PR now includes a bugfix and an enhancement; I can break them up if you'd like (the bugfix would have to be based on top of the enhancement). As it stands, I wonder if such a combination will confuse the automated release notes creation script...

[fingolfin]

LGTM

[ThomasBreuer]

Two details (one formulation and one changed requirement in a method installation) may cause problems, otherwise the changes are o.k.
In particular, I agree that we should open a new issue for collecting the problems with missing associativity checks in other domain constructions.

[ThomasBreuer]

This looks good, apart from a suspicious formulation (for me as a non-native speaker).