Downrank methods for Matrix and BaseDomain introduced in #2640 to unbreak Semigroups on master

[markuspf]

This addresses #2737 in that SemigroupsTestStandard() passes now.

An alternative solution would just rank down the appropriate methods for Matrix and BaseDomain until a fix is integrated into Semigroups.

The fix for Semigroups will likely involve taking most custom matrix code out, which was the long-term plan all along according to @james-d-mitchell. For that it would obviously be good if the MatrixObj code was suitably stable (at least by the time releases happen).

[markuspf]

Mhm. of course this also requires tests to be adapted. Maybe an optional down-ranking of the methods in question is a better approach?

[fingolfin]

I would simply down rank the methods now, and add a "FIXME: change this later" comment next to them. (And possibly also immediately open a PR which removes this comments, and marked as "do not merge", as a reminder that we need to do so; alternatively, an issue where we track this; might as well be issue #2737, too)

[fingolfin]

To be clearer: I would not bother adding this command line flag; just downrank all relevant methods by the same offset, so that the semigroups methods get up high enough. If that is sufficient to fix the problem, that is... I.e. if you know a reason why this won't work, please simply ignore me.

[markuspf]

I agree, let my try that...

[codecov]

Codecov Report

Merging #2754 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2754      +/-   ##
==========================================
+ Coverage   75.87%   75.87%   +<.01%     
==========================================
  Files         481      481              
  Lines      241310   241310              
==========================================
+ Hits       183098   183102       +4     
+ Misses      58212    58208       -4

Impacted Files	Coverage Δ
lib/matobj.gi	32.88% <ø> (ø)	⬆️
lib/matrix.gi	78.67% <ø> (ø)	⬆️
src/hpc/threadapi.c	43.18% <0%> (-0.11%)	⬇️
hpcgap/lib/hpc/stdtasks.g	64.7% <0%> (+1.27%)	⬆️

[ChrisJefferson]

One possible concern, the fact the methods still exist means if anyone starts using the MatrixObj code, their stuff will get "broken" if they load semigroups?

[markuspf]

I am tempted to say "don't do that then" right now ;)

If they write new code and want matrix objects in a particular filter, they can still make them. The Matrix functions are convenience constructors, anyway.

[james-d-mitchell]

I'm still tempted to just rename the methods in Semigroups, and then in time to remove them altogether. I will try to look at this this afternoon.

[james-d-mitchell]

I approve of this, as a temporary fix, provided that all of the Semigroups package tests run (pending), and the GAP tests run when Semigroups is loaded.

What I'd like to understand better is what a "proper" fix would look like. As far as I can see, the only proper fix for this is where Semigroups uses matrix objects (as it currently does) is for MatrixObjects should be a proper object (as per the comments I made in Issue #2737). Another option is for Semigroups to stop using matrix objects altogether. I'd like to know which of these options I should plan for.

[markuspf]

This breaks some tests of MatrixObj if Semigroups is loaded. Two options:

1. Comment out these tests
2. Ignore

I am all for 2, MatrixObjs are undocumented code.

[markuspf]

For completeness, with Semigroups loaded I get the following teststandard test failing:

########> Diff in /home/mp397/git/gap/tst/testinstall/MatrixObj/TraceMat.tst:6
# Input is:
m2 := Matrix(Integers,l);
# Expected output:
<2x2-matrix over Integers>
# But found:
Matrix(IsIntegerMatrix, [[1, 2], [3, 4]])
########
########> Diff in /home/mp397/git/gap/tst/testinstall/MatrixObj/TraceMat.tst:8
# Input is:
m3 := Matrix(GF(7),l*One(GF(7)));
# Expected output:
[ [ Z(7)^0, Z(7)^2 ], [ Z(7), Z(7)^4 ] ]
# But found:
Matrix(GF(7), [[Z(7)^0, Z(7)^2], [Z(7), Z(7)^4]])
########
########> Diff in /home/mp397/git/gap/tst/testinstall/MatrixObj/TraceMat.tst:12
# Input is:
TraceMat( m2 );
# Expected output:
5
# But found:
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `TraceMat' on 1 arguments
########
########> Diff in /home/mp397/git/gap/tst/testinstall/MatrixObj/TraceMat.tst:14
# Input is:
TraceMat( m3 );
# Expected output:
Z(7)^5
# But found:
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `TraceMat' on 1 arguments

i.e. two of them is are printing differences, and two are the non-existence of a TraceMat method.