Add `RandomMatrix`, `RandomInvertibleMatrix`; and fix a problem with a new feature for matrix groups by ThomasBreuer · Pull Request #6232 · gap-system/gap

ThomasBreuer · 2026-02-12T12:42:21Z

(resolves #6222)

add a description of the cache FULLGLNICOCACHE in the code; it is used only by NicomorphismFFMatGroupOnFullSpace
change the keys of FULLGLNICOCACHE: take the field of the matrix entries into account (not only its size), take the ConstructingFilter of the matrices into account
in NicomorphismFFMatGroupOnFullSpace, deal with special cases of matrices in IsBlockMatrixRep (which have no ConstructingFilter) and in Is8BitMatrixRep in the situation that the group itself lives over GF(2)
add Matrix calls in the function that computes preimages under an action homomorphism with Source a matrix group, in order to support matrix group elements of prescribed kinds of IsMatrixObj.
add tests for the new supported situations (most of the changes were actually forced by already available tests)
add \in methods for GL and SL groups where the first argument is in IsMatrixObj (such that no nice monomorphism is used in this situation)
introduce NormedRowVectors_internal( F, base ) and AsListOfFreeLeftModule_internal( F, base, zero ),
in order to admit IsVectorObjs in base without supporting the whole vector space machinery for these objects
introduce ExternalSet( G ) for matrix groups G, meaning the natural G-set (an undocumented method for permutation groups G was already available)
change NicomorphismFFMatGroupOnFullSpace such that we can act with the IsMatrixObj matrices on their IsVectorObj vectors, without unpacking the matrices
support IsMatrixObj matrices in the Random methods for GL and SL

(Some of the changes were forced by the fact that functionality for the intended new tests was missing.
In fact the list of known missing functionality is rather growing this way. Issue #6241 was also found just by trying to add tests. Until this problem is fixed, the current pull request deals only partially with the ideas from #6234.)

ThomasBreuer · 2026-02-12T14:27:46Z

repeated failures due to problems like
connection error: 12029 fetching http://mirrors.kernel.org/sourceware/cygwin/x86_64/setup.zst.sig

fingolfin

Unfortunately I also missed this one :-(

fingolfin · 2026-03-24T12:32:55Z

+           and ( IsSubset( F, BaseDomain( mat ) ) or
+                 ForAll( Unpack( mat ), row -> IsSubset( F, row ) ) );


Maybe add a comment that explains why the "or" case is needed? I.e., because the basedomain may be larger than necessary to represent elements, and we consider explicit conversion an option...

Though... thinking about it... how would one then "coerce" mat into an actual element of G (which can e.g. be multiplied with One(G) ) ? We don't have any tooling for that in GAP so far, do we? Because we don't need to, because all matrices (over a given common base field) are expected to be always "compatible" by some magic...

The real question is what \in for a matrix (object) and a matrix group shall mean:

Can the elements of the group be represented over different base domains?
If yes then arithmetic operations in the group are more expensive, if not then we are free to define a base domain for the group, and we can define \in such that the base domain of the matrix and the group must fit.

Concerning the coercion, I thought that Matrix( filt, R, M ) is intended for that purpose:
M is the given matrix which shall be turned into "a valid group element", R is the base domain of G or of One( G ), and filt is ConstructingFilter( One( G ) ).

fingolfin · 2026-03-24T12:37:36Z

+      m:= ZeroMatrix( d, d, m );
+      repeat
+        Randomize( rs, m );
+        det:= DeterminantMat( m );
+      until not IsZero( det );


Should we perhaps also have a method for RandomInvertibleMat which takes filt as well?

RandomInvertibleMat( rs, filt, d, F )

Currently RandomMat and RandomInvertibleMat aren't operations but plain functions.
So the default procedure will be to introduce tag based operations RandomMatrix and RandomInvertibleMatrix for which the new methods (with an optional filter or example matrix as argument) can get installed. (This is analogous to e.g. DiagonalMatrix.)
This way, we avoid the creation of intermediate objects.

ThomasBreuer · 2026-03-25T09:21:42Z

Two kinds of test failures:

Manual examples from the chapter about matrices must be adjusted; this is not surprising since this chapter contains now more examples using Random, so outputs involving Random calls likely change.
The Semigroups package provides already an operation RandomMatrix, which is not compatible with the RandomMatrix proposed for the GAP library. If Semigroups is loaded, the behaviour changes. I have to think about a solution.

Update on the second kind of failures:
The Semigroups package defines that RandomMatrix( R, n, k ) returns a square matrix with n rows and columns, and rank k, with random entries in R.
The current pull request wants to define that RandomMatrix( R, m, n ) returns a (perhaps not square) matrix with m rows and n columns, with entries in R.
These two definitions are clearly not compatible.
(The method from Semigroups has a higher rank, that's why we see the changed behaviour in the tests for the GAP library.)

ThomasBreuer · 2026-03-25T09:31:42Z

Another point:

Currently the determinant of a matrix computed with RandomInvertibleMatrix gets computed in the construction, but the result does not store this attribute.

The Random method for natural SL groups (currently the only customer of RandomInvertibleMatrix) needs the determinant, thus it gets computed again. Would an interface make sense that admits returning also the determinant?

These function conflict with forthcoming changes in GAP, and since they are undocumented, I'm just removing them in this commit. See: gap-system/gap#6232 semigroups#1158

These function conflict with forthcoming changes in GAP, and since they are undocumented, I'm just removing them in this commit. See: gap-system/gap#6232 #1158

These function conflict with forthcoming changes in GAP, and since they are undocumented, I'm just removing them in this commit. See: gap-system/gap#6232 semigroups#1158

These function conflict with forthcoming changes in GAP, and since they are undocumented, I'm just removing them in this commit. See: gap-system/gap#6232 #1158

- add a description of the cache `FULLGLNICOCACHE` in the code; it is used only by `NicomorphismFFMatGroupOnFullSpace` - change the keys of `FULLGLNICOCACHE`: take the field of the matrix entries into account (not only its size), take the `ConstructingFilter` of the matrices into account - in `NicomorphismFFMatGroupOnFullSpace`, deal with special cases of matrices in `IsBlockMatrixRep` (which have no `ConstructingFilter`) and in `Is8BitMatrixRep` in the situation that the group itself lives over `GF(2)` - in order to admit the action of `IsMatrixObj` matrices on vectors supported by GAP's `Enumerator`s of vector spaces, add a `\^` method that `Unpack`s the matrix object (Eventually we want to avoid this overhead, but then we need `Enumerator`s consisting of vector objects corresponding to the matrix objects.) - add `Matrix` calls in the function that computes preimages under an action homomorphism with `Source` a matrix group, in order to support matrix group elements of prescribed kinds of `IsMatrixObj`. - add tests for the new supported situations (most of the changes were actually forced by already available tests)

- introduce `NormedRowVectors_internal( F, base )`, and `AsListOfFreeLeftModule_internal( F, base, zero )`, in order to admit `IsVectorObj`s in `base` without supporting the whole vector space machinery for these objects - introduce `ExternalSet( G )` for matrix groups `G`, meaning the natural `G`-set (an undocumented method for permutation groups `G` was already available) - change `NicomorphismFFMatGroupOnFullSpace` such that we can act with the `IsMatrixObj` matrices on their `IsVectorObj` vectors, without unpacking the matrices - support `IsMatrixObj` matrices in the `Random` methods for GL and SL

and fix the `RankMat` method for `IsPlistMatrixRep`

Due to new examples in the chapter about matrices, the number of `Random` calls has increased, thus it is not surprising that some outputs involving such calls have changed.

ThomasBreuer · 2026-04-13T14:54:17Z

This pull request conflicts with the current version of the Semigroups package.
The problem will disappear when a Semigroups version containing the change from semigroups/Semigroups/pull/1163 is available.

(Alternatively, I can modify this pull request such that the changes to RandomMatrix are commented out.)

fingolfin · 2026-04-13T16:55:59Z

The testpackages testinstall-loadall tests suffered a timeout.

Before that, it also showed a failure (possibly only shows up after LoadAllPackages ?)

  ########> Diff in tst/testinstall/MatrixObj/RandomMatrix.tst:7
  # Input is:
  RandomMatrix( GF(9), 0, 1 );
  # Expected output:
  Error, Is8BitMatrixRep with zero rows not yet supported
  # But found:
  Error, no method found! For debugging hints type ?Recovery from NoMethodFound
  Error, no 1st choice method found for `RandomMatrixOp' on 3 arguments
  ########

and then at the end it times out in a test:

Mon, 13 Apr 2026 12:04:01 GMT  testing: tst/testinstall/gprdmat.tst
Mon, 13 Apr 2026 12:04:01 GMT       428 ms (207 ms GC) and 12.5MB allocated for gprdmat.tst
Mon, 13 Apr 2026 12:04:01 GMT  testing: tst/testinstall/grp/basic.tst
Mon, 13 Apr 2026 12:32:17 GMT  Error: The operation was canceled.

Also, most package build instantly, but 20 minutes to compile Semigroups :-/ (CC @james-d-mitchell though I don't think we can do much about that?)

fingolfin · 2026-04-13T16:56:28Z

I hope it will be all resolved by the new semigroups version then.

james-d-mitchell · 2026-04-13T17:47:53Z

I hope it will be all resolved by the new semigroups version then.

I have one ready to go will try to make a release soon

james-d-mitchell · 2026-04-13T19:02:15Z

The testpackages testinstall-loadall tests suffered a timeout.

Before that, it also showed a failure (possibly only shows up after LoadAllPackages ?)
  ########> Diff in tst/testinstall/MatrixObj/RandomMatrix.tst:7
  # Input is:
  RandomMatrix( GF(9), 0, 1 );
  # Expected output:
  Error, Is8BitMatrixRep with zero rows not yet supported
  # But found:
  Error, no method found! For debugging hints type ?Recovery from NoMethodFound
  Error, no 1st choice method found for `RandomMatrixOp' on 3 arguments
  ########
and then at the end it times out in a test:
Mon, 13 Apr 2026 12:04:01 GMT  testing: tst/testinstall/gprdmat.tst
Mon, 13 Apr 2026 12:04:01 GMT       428 ms (207 ms GC) and 12.5MB allocated for gprdmat.tst
Mon, 13 Apr 2026 12:04:01 GMT  testing: tst/testinstall/grp/basic.tst
Mon, 13 Apr 2026 12:32:17 GMT  Error: The operation was canceled.
Also, most package build instantly, but 20 minutes to compile Semigroups :-/ (CC @james-d-mitchell though I don't think we can do much about that?)

Right away not really, my plan longer term is to replace the kernel extension in Semigroups by Semigroups.jl, which will mean there's no compile time any more, but that Semigroups will only work in Julia. This doesn't really help, sorry :(

Suggested change is not feasible at this point, no reaction to follow-up requests

fingolfin · 2026-04-20T22:48:59Z

+    repeat
+      Randomize( rs, mat );
+      det:= DeterminantMat( mat );
+    until not IsZero( det );


Suggested change

until not IsZero( det );

until IsUnit( det );

Hrm, seems the existing RandomInvertibleMat also does it wrong (it only checks RankMat( mat ) = m)... though perhaps it is actually intentional, given that we also have RandomUnimodularMat, and the documentation for RandomInvertibleMat only states that "elements [are] taken from the ring R" but otherwise only claims the returned matrix is invertible; not that it is "invertible over R".

sigh

The example in the manual section for RandomInvertibleMat shows a matrix of integers whose inverse does not consist of integers. If we regard RandomInvertibleMatrix as an analogous function for matrix objects then a corresponding example would show a matrix object with the property that trying to invert it causes an error because the result would not fit to the given base domain. I can put this into the documentation.

On the other hand, we would be free to define the result of the new function RandomInvertibleMatrix to be a matrix (object) with determinant a unit in the given R.

So what do we want:
keep the proposed meaning of RandomInvertibleMatrix, analogous to RandomInvertibleMat (and add another function with the meaning "invertible over the given R"), or change the definition of RandomInvertibleMatrix?

RandomUnimodularMat is defined only for integer matrices, I think the name cannot be taken for the general case.
Note that RandomInvertibleMat works nicely for matrices over rings such as Integers mod n, because RankMat returns fail for matrices with non-unit determinants.

For the "old" code, one could at least defend this by saying: the matrix is invertible, just over a larger ring... and Inverse worked.

But I think the average would find it quite irritating if RandomInvertibleMatrix produced outputs for which Inverse would (or at least should) fail.

So I'd prefer if we used the IsUnit version.

(I am also puzzled why RandomInvertibleMat works the way it does? I can only think of two reasons:

it's an historic accident, where the code was written with with fields in mind, and after discovering the issue, it was decided to keep it (e.g. to avoid breaking other code)

someone wanted to have invertible rational matrices with all entries integers, and deemed this essential enough to warrant this otherwise surprising behavior.

Anyway: here we should do it differently, I'd say.

O.k., I will change the definition accordingly.

Since gap-system#6299, it is documented that all matrix objects in a matrix group must have the same `BaseDomain` value.

The result is guaranteed to be invertible over the prescribed `R`.

fingolfin · 2026-04-29T17:13:27Z

The PR looks fine to me. But it does a lot and the PR title is not quite adequate for that. I also don't think that release notes: not needed is appropriate given that this adds a bunch of new features, like RandomMatrix. I've changed the release notes PR label, and tweaked the title a bit. But it probably should be improved further, or maybe we should just have multiple entries...

ThomasBreuer added kind: bug Issues describing general bugs, and PRs fixing them topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Feb 12, 2026

ThomasBreuer mentioned this pull request Feb 13, 2026

MatrixObj: action homomorphisms for matrix groups whose elements are in IsMatrixObj not in IsMatrix #6234

Open

ThomasBreuer marked this pull request as draft February 18, 2026 10:38

ThomasBreuer force-pushed the TB_Nicomorphism_cache branch from 4328b09 to 021e71e Compare March 3, 2026 16:15

ThomasBreuer marked this pull request as ready for review March 4, 2026 09:01

fingolfin reviewed Mar 24, 2026

View reviewed changes

limakzi previously requested changes Mar 25, 2026

View reviewed changes

Comment thread lib/matrix.gi Outdated

ThomasBreuer mentioned this pull request Mar 26, 2026

signatures of RandomMatrix semigroups/Semigroups#1158

Closed

fingolfin closed this Apr 13, 2026

fingolfin reopened this Apr 13, 2026

fingolfin requested a review from limakzi April 13, 2026 07:56

ThomasBreuer added 10 commits April 13, 2026 13:16

add \in methods for matrix object and (GL or SL)

cf2c08b

make gaplint happy

8d3c3cf

adjust an error message in a test

a77ffb2

add some documentation

eb7be53

adjust also IsProjectiveActionHomomorphism methods

b97fa1d

remove a now unused local variable

a69aba8

add RandomMatrix, RandomInvertibleMatrix

f128e98

and fix the `RankMat` method for `IsPlistMatrixRep`

address a comment: use the new RandomInvertibleMatrix

e145c01

ThomasBreuer added 2 commits April 13, 2026 13:16

adjust manual examples

24920ca

Due to new examples in the chapter about matrices, the number of `Random` calls has increased, thus it is not surprising that some outputs involving such calls have changed.

fix the use of the "ConstructingFilter" option

53e33ff

ThomasBreuer force-pushed the TB_Nicomorphism_cache branch from 4cf7c57 to 53e33ff Compare April 13, 2026 11:31

ThomasBreuer closed this Apr 20, 2026

ThomasBreuer reopened this Apr 20, 2026

adjust to gap-system#6328

f685124

fingolfin reviewed Apr 20, 2026

View reviewed changes

ThomasBreuer added 2 commits April 22, 2026 09:45

update a comment in the code

ee41838

Since gap-system#6299, it is documented that all matrix objects in a matrix group must have the same `BaseDomain` value.

change the definition of RandomInvertibleMatrix

c5b44fb

The result is guaranteed to be invertible over the prescribed `R`.

ThomasBreuer requested a review from fingolfin April 22, 2026 12:22

fingolfin approved these changes Apr 29, 2026

View reviewed changes

fingolfin changed the title ~~fix a problem with a new feature for matrix groups~~ Add RandomMatrix, RandomInvertibleMatrix; and fix a problem with a new feature for matrix groups Apr 29, 2026

		and ( IsSubset( F, BaseDomain( mat ) ) or
		ForAll( Unpack( mat ), row -> IsSubset( F, row ) ) );

Conversation

ThomasBreuer commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThomasBreuer commented Feb 12, 2026

Uh oh!

fingolfin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomasBreuer Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ThomasBreuer commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThomasBreuer commented Mar 25, 2026

Uh oh!

ThomasBreuer commented Apr 13, 2026

Uh oh!

fingolfin commented Apr 13, 2026

Uh oh!

fingolfin commented Apr 13, 2026

Uh oh!

james-d-mitchell commented Apr 13, 2026

Uh oh!

james-d-mitchell commented Apr 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fingolfin commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ThomasBreuer commented Feb 12, 2026 •

edited

Loading

ThomasBreuer Mar 24, 2026 •

edited

Loading

ThomasBreuer commented Mar 25, 2026 •

edited

Loading