Tests fail with current GAP master

[zickgraf]

Steps to reproduce (taken from the package Modules):

LoadPackage( "Modules" );
ZZ := HomalgRingOfIntegers( );
M := HomalgMatrix( "[2,3,4,5,6,7]", 2, 3, ZZ );
M := LeftPresentation( M );
Z4 := ZZ / 4;
M4 := Z4 * M;
d := Resolution( 2, M4 );
dd := Hom( d, Z4 );
DD := Resolution( 2, dd );
D := Hom( DD, Z4 );
C := ZZ * D;

Expected result:

<A "complex" containing 2 morphisms of left cocomplexes at degrees [ 0 .. 2 ]>

Actual result:

Error, ElmWPObj: <pos> must be a positive small integer (not the integer 0) in
  ElmWPObj( R!.IdentityMatrices!.weak_pointers, NrColumns( C ) ) at /xxxxxx/pkg/homalg_project/MatricesForHomalg/gap/Tools.gi:1201 called from 
Eval( M ) at /xxxxxx/pkg/homalg_project/MatricesForHomalg/gap/ResidueClassRing.gi:259 called from
UnionOfRowsOp( M, RingRelations( HomalgRing( M ) ) ) at /xxxxxx/pkg/homalg_project/MatricesForHomalg/gap/ResidueClassRing.gi:287 called from
UnionOfRowsOp( mat ) at /xxxxxx/pkg/homalg_project/Modules/gap/BasicFunctors.gi:826 called from
CallFuncList( Functor!.OnObjects, arguments_of_functor ) at /xxxxxx/pkg/homalg_project/homalg/gap/HomalgFunctor.gi:791 called from
FunctorObj( Functor, [ obj1, obj2 ] ) at /xxxxxx/pkg/homalg_project/homalg/gap/HomalgFunctor.gi:1661 called from
...  at *stdin*:14
type 'quit;' to quit to outer loop

The first bad commit is gap-system/gap@1762043. The issue can also be triggered as follows:

LoadPackage( "RingsForHomalg" );
ZZ := HomalgRingOfIntegers( );
M := HomalgIdentityMatrix( 0, ZZ );
Eval(M);

[fingolfin]

Perhaps the wrong NrColumns method is being selected? Bad ranking of methods in packages was often hidden in older GAP versions due to a bug / misdesign in GAP, and the "first bad commit" you link fixed that, thus uncovering a load of bugs in packages, which we subsequently fixed by submitting PRs to those packages. But we mostly focused on testing released versions of packages....

[fingolfin]

incidentally, I wonder if my PR #273 miGht be relevant

[mohamed-barakat]

incidentally, I wonder if my PR #273 miGht be relevant

The error still persists with PR #273 tested against GAP 4.10dev-2239-geeb470f (latest commit).

I now believe that there is an error with the new GAP ranking: For some reason this method for Eval

https://github.com/homalg-project/homalg_project/blob/master/MatricesForHomalg/gap/Tools.gi#L1191

is ranked higher than this one

https://github.com/homalg-project/homalg_project/blob/master/MatricesForHomalg/gap/Tools.gi#L1272

@fingolfin: Any idea why?

[fingolfin]

@mohamed-barakat I noticed you applied a fix, but that fix just replaces one instable hack with another, and may very well break again. Indeed, looking at the two competing methods, one has rank RankFilter(IsHomalgMatrix and IsOne and HasNrRows and HasNrColumns) + 10 and the other has rank RankFilter(IsHomalgMatrix and IsZero and HasNrRows and HasNrColumns) + 20 (now changed to ... + 40. Based on that, it is completely unpredictable how these two methods will be ordered. Using hardcoded constants like your "10" and "20" simply does not scale, as the ranks of filters can change overtime, as packages get loaded which install implications that then raise the rank of these filters.

One way to resolve this here would be to replace the static rank adjustments with dynamic ones, using a no-arg function which returns a suitable rank adjustment, and which GAP calls again each time re-ranking happens. Then one can use something like this for the second method:

  {} -> RankFilter(IsHomalgMatrix and IsOne and HasNrRows and HasNrColumns)
         - RankFilter(IsHomalgMatrix and IsZero and HasNrRows and HasNrColumns)
         + 10

This feature is however only available in GAP 4.11; to write code that also works in older GAP versions, one needs to be a bit creative, e.g.:

if GAP < 4.11 then
  rank := 40
else
  rank := {} -> ...;
fi;
InstallMethod(Eval, ..., rank, ...)

That said, I wonder why homalg is using method dispatch there at all... Method dispatch is best suited for applications where it does not matter (other than in terms of performance) which of a list of given applicable methods are applied. If this is not the case here, I'd argue that you would be best of not using method dispatch, and instead e.g. merge the two methods into one (resp. turn them into two helper functions, then add a single method which basically calls either of the two helpers as appropriate).

[mohamed-barakat]

Thanks for the hint @fingolfin, I will reopen the issue.

[mohamed-barakat]

Can this issue be closed?