Problem with MutableCopyMatrix

[olexandr-konovalov]

A recent change in the GAP master branch broke several packages, see https://travis-ci.org/gap-infra/gap-docker-pkg-tests-master/builds/647932598, in particular aclib, polycyclic and others, e.g.

########> Diff in /home/gap/inst/gap-master/pkg/aclib-1.3.2/tst/manual.example\
-4.tst:4
# Input is:
IsTorsionFree(G);
# Expected output:
true
# But found:
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `MutableCopyMatrix' on 1 arguments
########

[fingolfin]

Backtrace of the above test, when run interactively:

gap> G:=AlmostCrystallographicPcpDim4("013",[8,0,1,0,1,0]);
Pcp-group with orders [ 2, 2, 0, 0, 0, 0 ]
gap> IsTorsionFree(G);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `MutableCopyMatrix' on 1 arguments at GAPROOT/lib/methsel2.g:249 called from
MutableCopyMat( x ) at GAPROOT/pkg/polycyclic/gap/cohom/onecohom.gi:91 called from
func( C[i] ) at GAPROOT/lib/coll.gi:665 called from
List( base, function ( x )
      return MutableCopyMat( x );
  end ) at GAPROOT/pkg/polycyclic/gap/cohom/onecohom.gi:91 called from
InverseCohMapping( coh, coh.gcc ) at GAPROOT/pkg/polycyclic/gap/cohom/onecohom.gi:127 called from
OneCohomologyEX( C ) at GAPROOT/pkg/polycyclic/gap/cohom/grpcom.gi:157 called from
ComplementClassesCR( C ) at GAPROOT/pkg/polycyclic/gap/cohom/grpcom.gi:296 called from
ComplementClassesEfaPcps( G, U, pcps ) at GAPROOT/pkg/polycyclic/gap/cohom/grpcom.gi:331 called from
ComplementClasses( K, N ) at GAPROOT/pkg/polycyclic/gap/pcpgrp/torsion.gi:216 called from
<function "IsTorsionFree for pcp groups">( <arguments> )
 called from read-eval loop at *errin*:1
brk> x;
[ 1 ]

So the argument to MutableCopyMat is in fact not a matrix, hence the error.
This looks like a bug in polycyclic that went unnoticed in the past due
to MutableCopyMat being more forgiving?

As a quick fix, we could add a suitable MutableCopyMat method to deal
with this case as before.

In the meantime, I can look into fixing polycyclic, and releasing it.
(But this might take me some time, hence we might want to consider
a quick fix now)
Then we can see if there are more issues or if this fixes all.
(If it does, drop the quick fix. Or never apply the quick fix to GAP)

CC @ThomasBreuer

[olexandr-konovalov]

Thanks @fingolfin. Of course, there are some other packages in https://travis-ci.org/gap-infra/gap-docker-pkg-tests-master/, and perhaps also in https://travis-ci.org/gap-infra/gap-docker-pkg-tests-master-staging/ - the latter would be less visible, since they are failing anyway, but just today I've seen the same issue in HAPcryst. We may need some help in fixing those packages too.

[fingolfin]

All these packages use polycyclic, so I would not be surprised if they all run into this very same bug. But we can just wait for my polycyclic fix to find out. Am on a crowded train now, though, more later or tomorrow

[ThomasBreuer]

@fingolfin
In GAP 4.10 (and in the stable-4.11 branch), the generic method for MutableCopyMat is installed with requirement IsList.
This method (which just returns the list of ShallowCopy results for the entries of the argument) is installed with requirement IsMatrix in the master branch.

I think that the new generic method in the master branch is o.k. and should be kept.

If the question is relevant for GAP 4.11, we could install additionally a (temporary) method for the requirement IsList; this method should print a warning that the argument is not a matrix, and then return the result.

(The only case for which MutableCopyMat is documented in GAP 4.10 is the cvec package, the relevant text from lib/matrix.gd has apparently never been added to the Reference Manual.)

[fingolfin]

In GAP 4.11, the recent MatrixObj change are not present, so it doesn’t matter in the context of this issue.

It does matter for GAP 4.12. In GAP 4.12, any code which (mistakenly) applied MutableCopyMat to a vector that is not a matrix is broken, including older polycyclic releases. While I will release a fixed version of polycyclic, of course I can’t take care of user code that I know nothing about.

However, the good thing about this issue is that if code contains this mistake, it should simply run into a break loop, instead of producing bad results, and then debugging that regression should be fairly straight forward. We might want to mention in the release notes that “MutableCopyMat was made more strict and no longer can be used to duplicate vectors (which it was never meant to do, but it worked by accident); if you encounter this as an issue, please change your code to use ShallowCopy instead” or something like that.

Of course we could also add back a method with filter IsList In order to keep legacy code working, but all in all I don’t think we really need to.

I don’t understand your last paragraph about IsMutableCopyMat, which I have never seen before. How does it relate to this issue?

[olexandr-konovalov]

#3895 helped only partially, see e.g. https://travis-ci.org/gap-infra/gap-docker-pkg-tests-master/builds/650295781 a day ago where tests for aclib, cryst, matgrp and numericalsgps still fail.

[ThomasBreuer]

@alex-konovalov I cannot reproduce the problem in aclib. Currently I can see an IsMatrixObj related problem in matgrp, but this is caused by (a missing method for) Unpack not MutableCopyMat.
The pull request #3899 deals with this problem.

[olexandr-konovalov]

@ThomasBreuer indeed, right now aclib tests pass in GAP master branch, see e.g. https://travis-ci.org/github/gap-infra/gap-docker-pkg-tests-master/jobs/702668191

[fingolfin]

I believe this is not an issue anymore