Tweak Rees matrix semigroups code

[fingolfin]

This removes the Matrix attribute; instead, methods for the Matrix operation
are installed, which delegate suitably to MatrixOfReesMatrixSemigroup resp.
MatrixOfZeroReesMatrixSemigroup, which now are independent attributes (instead
of both being synonyms for Matrix).

Also change Rees matrix semigroup code which access the attributes Rows and
Columns to instead access RowsOfReesZeroMatrixSemigroup, etc., explicitly, to
mirror the changes for Matrix, and to prepare for the eventuality that we
might want to convert Rows and Columns into plain operations, too.

Finally, tweak some manual entries for Rees matrix semigroups: mansections
referencing themselves should still use <Ref> for that.

The motivation for this is that we may want to allow using single-arg Matrix as a convenient way to construct MatrixObj instances; say Matrix(old_style_mat) could take a classic list-of-lists, and return an equivalent MatrixObj. That said, we might not actually do this; but simply the fact that Matrix is already an operation for 2, 3, ... args means that it is somewhat awkward to have it also be an attribute. So I'd rather avoid that.

Perhaps we should do the same for Rows and Columns, just in case we want to provide similar operations for MatrixObj? Not sure about that, though. And even if we over such APIs, we could call them, say, RowsOfMatrix; and then, perhaps we'd want an iterator anyway, so it'd be RowIterator/ColumnIterator; so I am not sure it's worth the hassle to even think about this more at this stage? The main motivation to change them, too, at this stage, would be symmetry; i.e., trying to not surprise people using Rees matrix semigroups by odd differences between Matrix on the one hand, and Rows/Columns on the other.

This PR should NOT be merged before Semigroups merges semigroups/Semigroups#618 and then has a release.

[ThomasBreuer]

Besides the specific comments in the code, I would like to emphasize that after the change, Matrix for Rees matrix semigroups will return an immutable result, whereas the result has been mutable up to now (due to a declaration of Matrix as a mutable attribute, which is read before the declaration in reesmat.gd).
I think the new behaviour is better, but the change may cause problems.

[ThomasBreuer]

Up to now, there was just one attribute Matrix with two synonyms MatrixOfReesMatrixSemigroup, MatrixOfReesZeroMatrixSemigroup.
Do we really need/want two independent attributes MatrixOfReesMatrixSemigroup, MatrixOfReesZeroMatrixSemigroup?

[wilfwilson]

What would be an alternative way of doing this, so that there were not two independent attributes? We could have one attribute called something like MatrixOfReesOrReesZeroMatrixSemigroup with the other names being synonyms for it, but that feels a bit weird to me.

[ThomasBreuer]

Since there was just one attribute with different names MatrixOfReesMatrixSemigroup, MatrixOfReesZeroMatrixSemigroup up to now, the situation seems to be that either no object lies in both IsReesMatrixSubsemigroup and IsReesZeroMatrixSubsemigroup or the attribute values for objects in both filters are equal.
In the former case, introducing two independent attributes is on the one hand safer (one gets an error if one calls the wrong attribute), but on the other hand Matrix methods for the two cases make the distinction disappear, and from this point of view just one filter is wasted.
In the latter case, introducing two independent attributes may cause that the same value has to be computed twice, if one asks for the values of both attributes.

[wilfwilson]

The former case is the true one: IsReesMatrixSemigroupElement and IsReesZeroMatrixSemigroupElement (from which IsReesMatrixSubsemigroup and IsReesZeroMatrixSubsemigroup are derived) are different categories, and so a GAP object cannot lie in both.

Is it a problem to 'waste' filters?

[fingolfin]

I asked @wilfwilson beforehand to make sure that the filters are mutually exclusive, and hence split this into two attributes. The alternative would IMHO not be to introduce MatrixOfReesOrReesZeroMatrixSemigroup, but rather I'd just turn one DeclareAttribute back into DeclareSynonymAttr.

But I really think it's best the way I now did it. Yes, we loose one filter bit. I don't see an issue with that, we have plenty bits left ;-).

[wilfwilson]

Thanks for pointing out the mutability difference. I would be surprised if the immutable result causes anything to break, but we'll see.

[fingolfin]

Besides the specific comments in the code, I would like to emphasize that after the change, Matrix for Rees matrix semigroups will return an immutable result, whereas the result has been mutable up to now (due to a declaration of Matrix as a mutable attribute, which is read before the declaration in reesmat.gd).

Good point, I actually forgot about that! I'll update this PR to also remove that "mutable attribute Matrix".

[james-d-mitchell]

Thanks for these changes and comments all, I will double check that everything still works in Semigroups today or tomorrow, then I’ll happy “approve”.

[fingolfin]

@james-d-mitchell thank you! In any case, we won't merge this before a new Semigroups release with the changes has been made, and picked up into the packages distribution.

[wilfwilson]

@james-d-mitchell made a new release (thanks!), hopefully it will be picked up soon by the package distribution system.

[fingolfin]

Excellent. So once @alex-konovalov resp. the package distribution picked up Semigroups 3.1.5, we could merge this, I guess. Well, of course only after rebasing it and rerunning the tests to make sure they then pass again.

[wilfwilson]

Would be fine by me.

[fingolfin]

Tests still fail, because the YangBaxter package does this: DeclareAttribute("Matrix", IsCycleSet);

[wilfwilson]

Which, for ease of keeping track, @fingolfin has made a PR to address at gap-packages/YangBaxter#64.

[wilfwilson]

Seeing as YangBaxter 0.9.0 has been released, I’m hoping that the tests will now pass.

[wilfwilson]

@fingolfin Could you rebase this please? Hopefully that will make the tests pass, and then we can merge 🙂

[coveralls]

Coverage increased (+0.0002%) to 84.514% when pulling 13e9d0f on fingolfin:mh/ReesMatrix into 70b459f on gap-system:master.