Start the process of making vector and matrix objects official

[ThomasBreuer]

• turned the raw documentation for vector and matrix objects into
  GAPDoc format, which is included in the GAP Reference Manual;
  this concerns also row list matrices (lib/matobjplist.gd);
  the manual chapter "Matrix Objects" now contains the relevant information,
  still some material will have to be moved between the chapters
  "Matrices" and "Matrix Objects"

• added a preliminary version of missing default methods;
  due to that, many tests from GAP's test suite now fail;
  apparently the 'IsMatrixObj' methods get higher rank than the
  methods for IsPlistRep matrices, note that some of the latter ones
  do not require IsMatrix but just IsListDefault;
  and note that some IsMatrixObj methods do not only override
  methods for IsPlistRep matrices but also methods for other
  wrapped objects which claim to be in IsMatrix, such as some basis objects
  and Lie objects

• default methods using 'Unpack' must make sure that they are not
  called with IsPlistRep arguments,
  in order to avoid recursion depth traps or segmentation faults;
  this is due to the decision that IsMatrix implies IsMatrixObj
  --it would be technically easier to separate the worlds of IsMatrix
  and IsMatrixObj, but I still think that having the implication is logically
  better.

• renamed some operations in matrix.gd, kept the old names;
  renamed also AddRowVector to AddVector, kept the old name

• removed the TraceMat method installed for IsList,
  which is taken over by the method for IsMatrixObj;
  perhaps more methods can eventually disappear this way

• some open items are listed at the end of the files
  matobj2.gd (one TODO, Backwards compatibility, Open items) and
  matobj.gi (Backwards compatibility)

• next steps before this stuff can be merged:

  • make the tests pass again; for that, I have to make more experiments
    with the default methods for IsMatrixObj

  • deal with the open items; some decisions are still to be made

  • add manual examples, perhaps with a type of matrix objects for which
    not more than the compulsory methods are installed

  • add generic tests for implementations of vector/matrix objects,
    similar to the ones in 'tst/teststandard/arithlst.g'

  • revisit the available implementations of vector/matrix objects
    in the GAP library and in packages

[stevelinton]

This may be something that's already been discussed to death and resolved, in which case fine, but I'm a bit concerned about the idea that equality of matrix objects ignores the BaseDomain. This isn't really a problem for equality itself, in that it only causes complications/delays when you test the equality of two matrix objects over different base domains, which good code shouldn't do very often. The problem is the implications of that for \< and hashing (and perhaps more things). Suppose m1 and m2 are matrix objects whose entries lie in GF(q) but which both happen to be represented in some packed format over GF(q^d). When you ask m1 < m2 the code will need to ensure that you get the same result as if you had compared the equal GF(q) matrix objects, which could easily mean switching from cheap binary comparison of compressed matrices to some complicated element-by-element comparison. The situation for hash functions is even worse.

For these kinds of reasons, I would prefer to keep matrix objects with different recorded BaseDomains (or even maybe in different representations) separate as far as \= is concerned. We can still have an operation to test element-by-element equality of matrix objects, of course, but maybe we can give it a different name.

[ThomasBreuer]

@stevelinton thanks for the comment.
For vector/matrix objects that are not in IsList, we are free to define equality in the way you suggest (except for problems with already existing code).
And defining equality as restrictive as possible is in the spirit of the idea behind vector/matrix objects that one wants to have support for "homogeneous situations".
I interpret your proposal as follows.

• Generic \= methods for two vector/matrix objects return true if and only if the two ConstructingFilter values are identical, the two BaseDomain values are equal, the lengths/dimensions are equal, and the entries are equal.
• There are no generic \< methods for two vector/matrix objects.

[fingolfin]

@stevelinton in general, no matter what we do, I believe firmly in the following: if one wants to use hashing, and e.g. a hashtable of matrices or vectors, then the only way to do this efficiently is by forcing all inputs objects to be in the same representation, period. That's what I do in my own orbit code, for example.

I guess this correspond to forcing the ConstructingFilter to be identical (but we haven't yet full specified ConstructingFilter, I think -- but it seems like the right direction, anyway)

[stevelinton]

@fingolfin I agree. But assuming we want some generic way to get at a hash function, we either have to define our hash functions (and data structures that use them) as not respecting equality (which is an option, but a departure from our usual approach, and not really an option for \<) or we need to narrow the definition of equality, which provisionally, I think I prefer.

@ThomasBreuer I think your interpretation is correct.

[ThomasBreuer]

What is missing in the documentation of ConstructingFilter?

[ThomasBreuer]

(as mentioned in the description of commit c1fcd23)
I have now changed the definition of equality for IsVectorObj and IsMatrixObj as suggested by @stevelinton.
More precisely, we have to distinguish the cases where the object in question is in IsList (such that equality is already defined) or not.
Note that this has nothing to do with the question whether objects are in IsPlistRep; for example, the function ConjugacyClassesOfNaturalGroup creates class representatives in IsPlistRep but compares them to the identity element of the group, which is likely to be in Is8BitMatrixRep.
(Eventually, such situations should be improved.)

[fingolfin]

@ThomasBreuer I took the liberty of adding a few commits to this PR. Of course they can be changed and squashed, but they made the tests pass for me locally (let's see how this fares on GitHub).

This PR also needs to be rebased.

Several points cropped up:

IsRowListMatrix versus IsList

IsRowListMatrix does not imply IsList, nor does its documentation claim that it does; it only says "[...] a matrix object [...] which admits access to their [sic] rows, via list access " (BTW, "their" should be "its", I think). We should clarify this: Either ...

1. change the definition to DeclareCategory( "IsRowListMatrix", IsMatrixObj and IsList ); and also change its documentation to say this explicitly;

2. or else, change the documentation to say explicitly that a IsRowListMatrix can be, but does not need to be, in filter IsList; and in that case, we should also document what methods it should support. (Right now, I think that besides operator [], it should also support Iterator, so that one can do for row in matrix do; though we probably would simply provide a default method for that, so [] would be required, and a Iterator implementation optional.

I prefer the second variant, as that gives us more flexibility in the future, and imposes less of a burden on implementors. Also, it goes hand in hand with my next point ...

Do we really want to "require" resp. support IsBound[] and Unbind[] for row list matrices?

Right now we have these in the library:

DeclareOperation( "IsBound[]", [ IsRowListMatrix, IsPosInt ] );
DeclareOperation( "Unbind[]", [ IsRowListMatrix, IsPosInt ] );

I would suggest that we simply remove them: If we go with option 1 above (change IsRowListMatrix to imply IsList) they become redundant; and if we go with option 2, I see no reason why one would require IsRowListMatrix implementations to provide them. I also would rather not encourage users of MatrixObj to use either. I think IsBound[] is mostly useless (just perform an explicit range check, that's much clearer). Only Unbind[] for removing the last row is mildly useful; but then using Remove does the same and is easier (compared Unbind(mat[NrRows(mat)]) vs. Remove(mat).

(Slightly off topic, but: I'd also consider adding generic operations for adding/removing rows and columns from/to matrices, which can be used and implemented (or not -- an implementation might raise an error if changing the row/column count is not permitted) regardless of their representation (in particular: not restricted to IsRowlistMatrix). Say, AddRow or InsertRow, and RemoveColumn, etc. But that's a separate issue)

Let's downrank generic methods?

I downranked several "generic" methods substantially, because they got called instead of the supposedly more general ones in some cases; e.g. \* for IsMatrixObj and IsOrdinaryMatrix had the same rank as its counterpart for IsBlockMatrix and IsOrdinaryMatrix, and thus could be called instead of it. Besides downranking that method (and several others), I also increased the rank of IsBlockMatrix. But perhaps we want to downrank fewer methods; or downrank them less; or downrank additional methods -- I didn't try to go through everything to decide that, I just downranked the method that caused me troubles, plus a few ones close by.

IsBlockMatrixRep vs. IsOrdinaryMatrix

OK, this could/should probably go into a separate PR (or issue): I noticed that many IsBlockMatrixRep methods actually are restricted to IsBlockMatrixRep and IsOrdinaryMatrix; e.g. Length, NrRows, ... -- but I don't understand why? From my POV, all of them except the \* method could be changed by dropping the IsOrdinaryMatrix filter, couldn't they? (That said, there also seems to be no easy way to create a matrix in filter IsBlockMatrixRep which is not an ordinary matrix, as BlockMatrix forces that filter).

[fingolfin]

Also: IsOrdinaryMatrix right now implies/requires IsMatrix. I guess it and IsLieMatrix ought to be changed to IsMatrixObj? But they are declared in arith.gd... Perhaps we need matobj1.gd even earlier? Or perhaps we need new IsOrdinaryMatrixObj / IsLieMatrixObj, which we declare in one of the matobj*.gd files, at which point we'd also install implications IsOrdinaryMatrixObj => IsOrdinaryMatrix and IsLieMatrixObj => IsLieMatrix ?

[fingolfin]

It would be good to not let this bitrot further. I just rebased this PR, as there were several conflicts already. I also added a few more tweaks to it.

Perhaps we can at least merge parts of it already now, and focus on the remaining issues afterwards?

[ThomasBreuer]

@fingolfin How do you want to proceed, in order to merge parts of the changes? (I must admit that I got lost after your intervention in September.)

[fingolfin]

@ThomasBreuer I am sorry I lost you with my "intervention" (?) in September. I didn't think that the commits I added (which I mostly added to resolve many of the remaining test suite failures) would cause confusion. Please next time I loose you like that, don't hesitate to speak up immediately. I actually only added those commits back then because there hadn't been activity for 2-3 weeks and I didn't want this to bitrot; and similar I finally rebased yesterday because I didn't get a reply for two months.

Anyway, to proceed, I see two ways (not mutually exclusive):

• extract parts of these changes which can be merged right away and do so (e.g. some of my commits, like "Use mat[i,j] syntax some more"; perhaps also parts of your two commits, but since they do a lot at once, that's more difficult)

• get this PR to fully pass the test suite, possibly resolve a few more things (e.g. see the questions I raised in my comment from September 25; while I addressed some of them partially via commits I added to this PR, those should be considered preliminary, just as a means to get some tests to pass; I am happy to ditch them and start over, if we agree on a different solution!). Then merge this into master as soon as possible, so it gets tested thoroughly during the next couple months.

[fingolfin]

For now, I'll move some of my changes in here to separate PRs, then we (and others) can discuss them there and merge/revise/reject them there.

[fingolfin]

With cvec loaded, we get test failures all over the place, e.g.:

gap> t:= CharacterTable( PSL(2,5) );; IsCharacterTable( t mod 3 );
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `NewMatrix' on 4 arguments at GAPROOT/lib/methsel2.g:249 called from
NewMatrix( ConstructingFilter( example ), BaseDomain( example ), nrCols, list ) at GAPROOT/lib/matobj.gi:354 called from
Matrix( [  ], NumberColumns( m ), m ) at GAPROOT/pkg/cvec-2.7.4/gap/linalg.gi:61 called from
EmptySemiEchelonBasis( m ) at GAPROOT/pkg/cvec-2.7.4/gap/linalg.gi:1049 called from
MinimalPolynomialOfMatrixMC( m, 0, indet ) at GAPROOT/pkg/cvec-2.7.4/gap/linalg.gi:1293 called from
MinimalPolynomial( r, e, 1 ) at GAPROOT/lib/ringpoly.gi:621 called from
...  at *stdin*:3
type 'quit;' to quit to outer loop

[fingolfin]

This change swaps the name of the kernel operation and its synonym. What is the motivation for this? (I don't see it mentioned in the commit message).

This changes e.g. backtraces that involve [,] (see the third commit in this PR, where I adjust testspecial for that, among other things). Also note that we for [], we do this (and that was my main motivation for doing DeclareOperationKernel("[,]", ...)):

DeclareOperationKernel( "[]",
    [ IsList, IS_INT ],
    ELM_LIST );

[coveralls]

Coverage decreased (-0.01%) to 84.763% when pulling 79155a3 on ThomasBreuer:TB_MatrixObj_documentation into cf223e5 on gap-system:master.

[fingolfin]

@ThomasBreuer I revised my changes a couple more times now, and hope they are now simple enough. All Travis tests pass now. But I suspect we need to downrank more methods in order to make package test suites pass.

I was also wondering again if making IsMatrix imply IsMatrixObj is such a great idea after all. Of course it makes all kinds of transition easier, but it also means that code which asks for IsMatrixObj can never be sure it gets a genuine matrix object, or "just" a plain lost IsMatrix plist-of-plist. I think @james-d-mitchell (and perhaps also @wilfwilson ) were genuinely concerned about that as well, as it'd causes all kinds of issues in their code.

Makes me wonder: Did we ever consider the possibility of adding a filter IsMatrixOrMatrixObj which both IsMatrix and IsMatrixObj imply, instead of the IsMatrix => IsMatrixObj implication, and then using that for methods that are supposed to work on both? Not saying this is great either, but it has a different set of tradeoffs.. Hm

[wilfwilson]

I don't think I have any objections to IsMatrix implying IsMatrixObj, I think the Semigroups package can handle this fine.

[stevelinton]

I've been trying to use this PR to make the matrix obj wrappers for meataxe64 matrices work. I've implemented all the methods mentioned in section 26.13, but there seems to be quite a bit more needed. I've added multiplication methods and it seems I also need Unpack before any default methods will work.

Another problem -- IsOrdinaryMatrix implies IsList

[fingolfin]

@stevelinton I really would like to see this merged rather sooner than later. As it is, it once again needs to be rebased.

In retrospect, I am not sure it was a good idea to make such a big change in one go. At the very least not when it takes so long to get merged...

@ThomasBreuer I am kind of waiting for you to react to my last comment(s). It would be great (also for OSCAR) if we could make progress on MatrixObj in general, and this PR in particular.

[stevelinton]

@fingolfin I'd be happy to see this merged as is. My observations from trying to use it an sensibly be dealt with in a follow-up.

[ThomasBreuer]

@fingolfin @stevelinton I think we can proceed by "merging as is", and afterwards try to get rid of the explicit downrankings by SUM_FLAGS.
I have looked at the individual instances, my proposals for the second step would be as follows.

• \+, \-, \*:
  Instead of downranking the method requiring IsMatrixObj, we can install the method for lists a second time, with requirement IsMatrix for both arguments.

• \/:
  There is only a very general (= low rank) method not even requiring IsList.
  Again, we could install the same method with requirement IsMatrix for the first argument, and the method for IsMatrixObj is ranked below.

• MutableCopyMatrix and Length:
  The reason for downranking the method requiring IsMatrix is that
  other methods (for a gf2 matrix etc.) have too low rank (the rank of IsGF2MatrixRep);
  these methods should better require also IsMatrix,
  then this downranking can be omitted, as well as the one for the method requiring IsMatrixObj.

• Characteristic:
  In the method installation requiring IsVectorObj (which delegates to BaseDomain), we could require HasBaseDomain.

• ConstructingFilter:
  We have to make sure that the method for IsRowVector has lower rank than the method for an 8bit vector; for that, better install the latter one with additional requirement IsRowVector.

[fingolfin]

@ThomasBreuer @stevelinton I've now rebased this. From my point of view, it'd be fine to go along with @ThomasBreuer proposal: merge this now, then do as he suggested and replace the down ranking by doubling certain method installations

[fingolfin]

Approved -- @stevelinton if you are happy with it, please also consider approving.

We should of course wait for CI tests to complete before merging this.

[stevelinton]

As discussed at length. This is a step forward and breaks nothing we know about.