Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TransposedMatDestructive works neither for compressed nor for immutable matrices #303

Closed
fingolfin opened this issue Oct 24, 2015 · 3 comments
Labels
kind: bug Issues describing general bugs, and PRs fixing them

Comments

@fingolfin
Copy link
Member

The following was reported (in slightly different form) on the GAP forum:

gap> M:=NullMat(4,6,GF(5));
[ [ 0*Z(5), 0*Z(5), 0*Z(5), 0*Z(5), 0*Z(5), 0*Z(5) ], [ 0*Z(5), 0*Z(5), 0*Z(5), 0*Z(5), 0*Z(5), 0*Z(5) ],
  [ 0*Z(5), 0*Z(5), 0*Z(5), 0*Z(5), 0*Z(5), 0*Z(5) ], [ 0*Z(5), 0*Z(5), 0*Z(5), 0*Z(5), 0*Z(5), 0*Z(5) ] ]
gap> ConvertToMatrixRep(M);
5
gap> TransposedMatDestructive(M);
Error, Unbind of entry of locked compressed vector is forbidden in
  Unbind( mat[j][m + i] ); on line 1957 of file /BLABLA/gap/lib/matrix.gi called from
<function "unknown">( <arguments> )
 called from read-eval loop at line 44 of *stdin*
You can `return;' to ignore the assignment
brk>

Going beyond that, there is another issue with TransposedMatDestructive: Its documentation states the following:

TransposedMatDestructive( mat ):
If mat is a mutable matrix, then the transposed is computed by swapping the entries in mat. In this way mat gets changed. In all other cases the transposed is computed by TransposedMat (24.5-6).

So this suggests that TransposedMatDestructive should work for immutable matrices, but it also doesn't:

gap> M:=NullMat(4,6,GF(5));;
gap> MakeImmutable(M);;
gap> TransposedMatDestructive(M);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `MutableTransposedMatDestructive' on 1 arguments called from
<function "HANDLE_METHOD_NOT_FOUND">( <arguments> )
 called from read-eval loop at line 62 of *stdin*
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk>
@olexandr-konovalov olexandr-konovalov added the kind: bug Issues describing general bugs, and PRs fixing them label Oct 24, 2015
@fingolfin
Copy link
Member Author

Several solutions come to mind for first problem:

  • implement the operation for compressed matrices in an efficient way;
  • implement it, but in an inefficient way (e.g. using conversions to/from another format)
  • change the documentation to state that this is not supported for compressed matrices.

I prefer the first option, but it is the most work, of course.

For the second issue, we could:

  • implement the operation for immutable matrices by delegating to TransposedMat, as the documentation states;
  • change the documentation to state that TransposedMatDestructive is only provided for mutable matrices.

I prefer the second option, as it doesn't make sense to me to allow TransposedMatDestructive for immutable matrices, and I think doing so just promotes inefficient coding. But we should only do so if TransposedMatDestructive really never worked for immutable matrices, i.e. if this is not just a recent regression. (The reason for this is simple: If it never worked, it is safe to change the documentation; but if it used to work, we may have broken people's code, and I am conservative about doing that).

@stevelinton
Copy link
Contributor

For compressed (and non-square) matrices in the built-in GAP format it is not really possible to
transpose in-place. The reason behind this is a nasty kludge around the possibility of rows being shared between multiple compressed matrices.

Since TransposedMatDestructive doesn't promise that the change will be in-place, merely allows it,
it probably makes sense to install methods for that case (and the case of an immutablke matrix) that simply defer to TransposedMat where the appropriate methods all exist.

@stevelinton
Copy link
Contributor

This issue is subsumed by #306. I would close it if I could see how.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them
Projects
None yet
Development

No branches or pull requests

4 participants