WIP: Make integer and finite field matrices be MatrixObjs

[wilfwilson]

I'm trying to see how painful it is, and whether it's even possible, to make Semigroups matrices fit into the GAP MatrixObj framework. This would let us benefit from the setup there, in particular some new notation, and default methods, and we would also be using the standard names for things, so that people familiar with how to manipulate other matrices in GAP will already know what the appropriate commands are for our matrices.

(One thing that I like is that with matrix objects, you can do mat[i,j] to access the entry in row i and column j.)

This is related to but still mostly independent of #512: we can create matrices with some other operation than Matrix, but still have them be matrix objects. However, if we can make our matrices fully fit into the MatrixObj framework, then I think we would happily continue to use Matrix.

On the whole this seems to be going fairly smoothly, with no major interventions, except for the requirement of GAP 4.10.0 rather than GAP 4.9.0. However, I've definitely not done everything (I'm not sure exactly which methods MatrixObjs need to offer), but perhaps the stumbling block will be if it is required to implement BaseDomain methods for our matrices.

Basically, the BaseDomain is supposed to be something like the field/ring/whatever generated by the elements of the matrix, or at least the field/ring/whatever in which they live. However, I'm not sure if we can always do that. Consider an NTPMatrix: the entries are integers, but we definitely wouldn't want the BaseDomain to be Integers; but we also haven't implemented NTP semiring objects (and we have no intention to), so we couldn't return an appropriate NTP semiring either. Therefore we can't install a BaseDomain method in this case. Hopefully that's not fatal.

[james-d-mitchell]

I support this, will be interested to see how it works out!

[james-d-mitchell]

@wilfwilson is this finished? Or is there more work to do? If so, what is the work required?

[wilfwilson]

No, it's not finished. In this PR, I have successfully made all of our matrices have the filter MatrixObj, and our tests still work and I don't think we break GAP's tests. But I am yet to work out fully whether it is appropriate to make such a blanket change.

I've had some discussions with @fingolfin about whether it is acceptable for matrices in MatrixObj to have entries (i.e. what you get by accessing mat[i,j]) that are not actually in the semiring that they're supposedly in (i.e. the semiring that you would expect to get if you called BaseDomain). For example, the entries of one of our NTPMatrix objects are either integers or infinity, and in particular, they are not elements of a tropical semiring. (The operations are different.)

For our Semigroups-related purposes this doesn't matter, because we know to always treat them as if they are in the correct semiring. But if our objects are MatrixObj, then there are other methods that could apply, e.g. general library methods, and these might produce nonsense if they assume the entries of matrix are in the correct semiring. For example, a default TraceMat method that would sum the entries along the diagonal would not always work for our NTPMatrix objects, for example. (And even if it did return the expected thing, it would still be returning either an integer or infinity, which is still not actually an element of the semiring...)

My impression was that Max isn't keen. His suggestion was to implement dummy semiring objects and elements, so that BaseDomain could be this dummy semiring, and the entries of the matrix (what you get by accessing mat[i,j]) would be elements that belong to this semiring. I don't think I can be bothered doing that. Maybe you would be, or you have other thoughts, @james-d-mitchell? We'd still be able to construct our matrices in the current way, and access the integers/infinities directly via some other method, but if you interface with the MatrixObj API you get actual 'proper' appropriate objects.

Note that for our matrices over finite fields and our matrices over integers, there are no problems since these fields/rings are implemented in GAP. Therefore, I can and will make these types be MatrixObjs. So if nothing else, I think we should do that (but I would need to separate those changes from the other changes if that's all we're going to do).

These are the realistic options I see for the type-problem thing:

1. We don't have the 'problematic' kinds of matrices lie in IsMatrixObj. Easiest option, but then we get no MatrixObj benefit.
2. We don't care about the potential problem, and we live with the fact that other MatrixObj methods might mess when given our matrices. I think this is reasonable, as long our our manual contains sufficient warnings, and a user can't accidentally call our matrix construction functions. If it's possible that our "non-standard" methods become the highest ranked methods for matrix-construction operations that a non-Semigroups interested person might reasonably want to perform, then I think that would be bad. One option would be to make our matrix-creation functions be MatrixOverSemiring - but I would prefer to avoid that and stick with Matrix.
3. We make sure that any generic library MatrixObj methods that don't work properly for our matrices have higher-ranked methods in our package that do do the "right" thing, or at least do something appropriate, like given an error. This way, in theory, even if a user accidentally creates a matrix via the Semigroups package, no one will get wrong results.

In reality, however, I don't see a user accidentally using our methods in a way that will produce these "weird" matrices. These matrices can only be created if you explicitly pass the first filter IsNTPMatrix, IsBooleanMat, and so on, which are filters that only exist in the Semigroups package. Therefore I think my preferred option is 2., with a reasonable amount of option 3. In particular, I would make BaseDomain for one of our IsMatrixOverSemiring matrices raise an ErrorNoReturn("not yet implemented"); error, and I'd investigate what other such changes I'd make.

What do you think? None of this is going to require much code to change, the question is just being careful to make sure we don't surprise anyone or break anything.

[fingolfin]

One thing that is not yet quite clear to me is: What would be the "IsMatrixObj benefit" for e.g. your tropical matrices? My impression is that virtually everything for it has to be coded from scratch, there seems to be zero possibility of "code reuse" there, or at least not for anything that ever touches entries, because the entries are integers but with different "addition" and "multiplication".

I think making this clearer might help in arriving at a good plan

[james-d-mitchell]

Ideally we'd move to a situation where we have Semirings code somewhere, and then we could use the generic MatrixObj code in the GAP library to replace the existing code in the Semigroups package for tropical matrices etc. I think at some point @markuspf and I and Mark Kambites started writing a basic Semirings package. I perhaps can dig that out. Does this sound like a reasonable way forward?

[wilfwilson]

Not unreasonable, but I don't think it's worth doing simply for the sake of nicely fitting into the MatrixObj framework. If making a Semirings package is something you want to do independently for more interesting purposes, then perhaps it would make sense to use those elements as the entries of our Semigroups package matrices. But otherwise, I think that's a lot of work for very little benefit.

For now, I'll (eventually) update this PR to implement just IsMatrixObj for the easy types as discussed above, and try my best to make sure that Semigroups remains as compatible with GAP and other packages as possible.

[wilfwilson]

I have split this PR into two commits, so you can see what are the 'important' changes and what are just the ones taking advantage of the mat[x][y] syntax.

There are two main thrusts to the work:
Make sure that when the Semigroups package is asked to create a matrix via Matrix...

1. ....and the matrix is over the integers or over a finite field and the arguments make sense for the Semigroups package, then it should return an objects that fulfils the MatrixObj spec.
2. ...and the requested matrix doesn't make sense for the Semigroups package (e.g. is non-square, or over a semiring that we don't care about), then it should TryNextMethod() (and hence dispatch to GAP's methods) rather than give an error.

This PR is not complete in either sense but it goes some way to achieving this (it requires more testing of Semigroups and GAP to see what is lacking).

@james-d-mitchell Please feel free to either take over this PR, or be inspired by what I have (partially) done in your own implementation.

[fingolfin]

Switching to TryNextMethod is precisely what PR #819 does :-)

[fingolfin]

The changes of me that were merged to stable-4.0 will likely conflict with this PR in parts, though I hope the conflicts will be easy enough to resolve.

I wonder how to proceed with this, though. One idea would be to simply merge it (after resolving the conflicts mentioned above), and go with it: sure the MatrixObj implementation may not be complete, but I don't think there are serious downsides to a partial one at this point?

Alternatively, this could wait until a proper "test suite" for MatrixObj is available... but of course we've been waiting for one for years :-/. Then again, there are currently GAP Days being organized for later this year where the idea is to work on MatrixObj, so perhaps such a test suite could be developed there, and perhaps even co-developed with this PR here...

@james-d-mitchell if you are interested in principle, there is a Doodle on the GAP Slack in channel #gapdays where one can express a preferences between two potential weeks for this

[james-d-mitchell]

@fingolfin I'd like to attend the next GAP days if I can, I'm going to try to resurrect this PR just now, and see to what extent it resolves the issues in

gap-system/gap#4878

[wilfwilson]

I've rebased, although I didn't properly verify that the result of the rebase made sense. I'm not sure what, if anything, is worth salvaging from here (hopefully something!). But please feel free to do what you like with it.