Replace IsIntegerMatrix and IsMatrixOverFiniteField by MatrixObj

[james-d-mitchell]

This PR so far contains changes that removes the code for integer matrices in the Semigroups package and replaces them with MatrixObj. This is WIP, and shouldn't be merged, at least in part because I think there's no released version of GAP that this will work with, and it's a backwards incompatible change.

[james-d-mitchell]

I believe that this resolves the original issue raised in #807

[james-d-mitchell]

I've adjusted this again to remove the changes to the matrices over finite fields code, and also to try to make this work with GAP master and 4.11 and 4.10. I've tested this with 4.11 and it works ok.

[james-d-mitchell]

Slight revision, I don't currently expect this to work with master because there's more work to do to replace matrices over finite fields.

[james-d-mitchell]

I have now replaced the code in Semigroups for matrices over finite fields, and I expect that the Semigroups package tests will pass with GAP master, not sure about the other versions of GAP.

[james-d-mitchell]

@fingolfin @wilfwilson the CI seems to be broken again with a 404 for

https://www.gap-system.org/pub/gap/gap4pkgs/packages-stable-4.10.tar.gz

in

https://github.com/semigroups/Semigroups/runs/7041559173?check_suite_focus=true

[wilfwilson]

That link seems to be resolving now.

[fingolfin]

Broken CI was due to migration of the GAP website; I forgot to restore on HTTP redirect rule... fixed now

Now I've done the following: I took a GAP master build with the latest package distro, and replace semigroups in there with what is in this branch. I then run parts of the GAP test suite, after first loading Semigroups (I did not yet try "all" combinations).

This revealed a minor test regressions:

########> Diff in /Users/mhorn/Projekte/GAP/gap/tst/testinstall/magma.tst:54
# Input is:
AsSemigroup( F );
# Expected output:
<semigroup of size 6, with 2 generators>
# But found:
<semigroup of size 6, 2x2 matrices over GF(2) with 2 generators>
########

but that looks like something we can easily adjust in the GAP test suite (e.g. by suppressing the print output; we can't just change the expected output, though, as it now differs depending on whether semigroups is loaded or not...)

I am now going to run GAP's tst/teststandard.g and tst/testextra.g

[james-d-mitchell]

Thanks @fingolfin, we are running some GAP tests in the jobs on azure, but these are not being run here (they use GAP 4.10 and 4.11). I think the best solution to that change is to suppress the output, as you say.

[james-d-mitchell]

Is there a gh action for running the GAP library tests anywhere?

[james-d-mitchell]

Seems like gh-actions is having a "major outage".

[fingolfin]

Is there a gh action for running the GAP library tests anywhere?

No, but assuming you already have a running GAP, then normally you can run the (minimal) test suite via Read(Filename(DirectoriesLibrary("tst"), "testinstall.g"));. This will exit the current GAP with an exit code set to indicate success or failure.

[james-d-mitchell]

Thanks @fingolfin, I think I'm just a little confused about how to do this in the context of the gh actions for GAP, since there's nowhere in the ci.yml file that we actually invoke GAP directly, just use one of the GAP gh actions. If you could point me in the right direction that'd be awesome.

I am nearly finished with this, I need to add a few more tests, and rebase the PR, but otherwise everything is looking good!

[fingolfin]

@james-d-mitchell it should be possible to run gap by just invoking it as such in a bash shell script. So something like this should work:

      - name: "Run GAP testinstall test suite"
        run: gap -c 'Read(Filename(DirectoriesLibrary("tst"), "testinstall.g"));'

[fingolfin]

Ah OK, I see you also got it to work by hacking gap-actions/run-pkg-tests. Also fine. Perhaps even better, to ensure the right version of semigroups is being loaded? Anyway, it seems to work.

[james-d-mitchell]

Thanks @fingolfin, yup I figured it out, thanks for letting me know.

I've tried as far as possible to not require any changes to the GAP tests for this version of Semigroups, but there are some I can't workaround (without doing too much work), so I'll likely submit a PR to GAP for this.

[fingolfin]

Great. And of course if there are changes needed on the GAP side, that's fine (and kinda to be expected). Let me know if I can help with anything, or if a chat would help at any point.

[james-d-mitchell]

@fingolfin I think the only changes required in GAP master are in the PR:

gap-system/gap#4971