Taking an inverse of an integer matrix throws an error instead of returning `fail`

[reiniscirpons]

Consider the following 3 examples:

gap> A := Matrix(Integers, [
> [1, 2, 3],
> [4, 3, -1],
> [1, 0, -3]]);
<3x3-matrix over Integers>
gap> B := Matrix(Integers, [
> [1, -1, 0],
> [0, 0, 1],
> [1, 0, 1]]);
<3x3-matrix over Integers>
gap> C := Matrix(Integers, [
> [0, 0, 0],
> [0, 1, -1],
> [1, 0, 1]]);
<3x3-matrix over Integers>
gap> A^-1;
Error, the elements in <list> must lie in <basedomain>
*[1] Error( "the elements in <list> must lie in <basedomain>" );
   @ /Users/rcirpons/Desktop/Source/gap/lib/matobjplist.gi:90
 [2] MakeIsPlistVectorRep( v![1], list, true )
   @ /Users/rcirpons/Desktop/Source/gap/lib/matobjplist.gi:319
 [3] Vector( list[i], t )
   @ /Users/rcirpons/Desktop/Source/gap/lib/matobjplist.gi:687
 [4] Matrix( n, Length( n ), M )
   @ /Users/rcirpons/Desktop/Source/gap/lib/matobjplist.gi:1215
<function "InverseSameMutability [ IsPlistMatrixRep ]">( <arguments> )
 called from read-eval loop at *stdin*:160
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk> quit;
gap> Display(B ^ -1);
<3x3-matrix over Integers:
[[ 0, -1, 1 ]
 [ -1, -1, 1 ]
 [ 0, 1, 0 ]
]>
gap> C^-1;
fail

Inverting A throws, however inverting B succeeds and inverting C fails but does not throw.
The error for A seems to be thrown since A is invertible over the rationals but not the integers, and the
method seems to be trying to cast the rational inverse to an integer matrix, which understandably fails.

I think the correct behavior would be to simply fail if the inverse is not an integer matrix. This can be checked rather easily, since
an integer matrix is invertible with an inverse lying in the integers precisely when the determinant is +1 or -1. So probably adding
this check prior to trying to invert, and returning fail if it is not +1 or -1 is the simplest fix. At the very least we should document this behavior if this is intended.

[james-d-mitchell]

Thanks for the report, I'm not sure this is an issue in Semigroups (but it might be), I think there's a GAP issue I opened on exactly this topic, patchy internet right now, so have difficulty searching for it.

[james-d-mitchell]

The issue I mentioned above is:

gap-system/gap#4884

which says that the issue was resolved (and I guess will be part of GAP 4.16)?

[reiniscirpons]

@james-d-mitchell I guess the resolution is to make the inverse throw an error. I think we should add this to the docs then.

[james-d-mitchell]

Some further analysis:

• for A in your examples, it seems that the inverse is found (and contains rationals) but then the function that tries to make this into an "integer" matrix throws an error.
• B seems to behave as intended
• C gets caught because it has determinant 0 (I guess, didn't verify this)

So probably somewhere in GAP there's a check that the determinant of the matrix which is being inverted is not 0, and fail is returned then, but that check isn't completely appropriate for integer matrices.

[james-d-mitchell]

Also seems that adding

diff --git a/lib/matobjplist.gi b/lib/matobjplist.gi
index 4e2e188cd..4bc8323d8 100644
--- a/lib/matobjplist.gi
+++ b/lib/matobjplist.gi
@@ -1172,7 +1172,8 @@ InstallMethod( InverseImmutable,
   [ "IsPlistMatrixRep" ],
   function( M )
     local n;
-    if M![RLPOS] <> Length(M![ROWSPOS]) then
+    if  M![RLPOS] <> Length(M![ROWSPOS]) or (IsIntegers(BaseDomain(M))
+and not Determinant(M) in [-1, 1]) then
         #Error("InverseMutable: Matrix is not square");
         #return;
         return fail;
@@ -1188,7 +1189,8 @@ InstallMethod( InverseSameMutability,
   [ "IsPlistMatrixRep" ],
   function( M )
     local n;
-    if M![RLPOS] <> Length(M![ROWSPOS]) then
+    if  M![RLPOS] <> Length(M![ROWSPOS]) or (IsIntegers(BaseDomain(M))
+and not Determinant(M) in [-1, 1]) then
         #Error("InverseMutable: Matrix is not square");
         #return;
         return fail;

fixes this so that both A and C return fail when inverting is tried, but apparently Determinant isn't declared at this point so can't be used like this (the patch still works, just has warnings on start up).