fix ConvertToMatrixRep, fix some Matrix calls#6396
Conversation
ThomasBreuer
added
kind: bug
|
Unfortunately this PR does not fix gap-packages/recog#472 |
This is interesting. I will have a look at it. |
For a matrix or matrix object, it is safe to call `BaseDomain` if `HasBaseDomain` is set. For matrices in `Is8BitMatrixRep`, it may happen that `BaseDomain` for the matrix and some of its rows differ. Thus try to avoid computing `BaseDomain`.
Up to now, we ran into an error if some rows of the matrix were already compressed w.r.t. an unwanted `q`.
|
With the current version of this pull request, |
| else | ||
| R:= f; | ||
| fi; | ||
| #TODO: Here `BaseDomain( G )` should be used. |
There was a problem hiding this comment.
| #TODO: Here `BaseDomain( G )` should be used. | |
| #TODO: Here `BaseDomain( G )` should be used, once it is implemented |
|
|
||
| # try to find a vector that has nonzero coefficients for all b | ||
| binv:= Inverse( ImmutableMatrix( f, Matrix( b, Grep ) ) ); | ||
| binv:= Inverse( ImmutableMatrix( f, Matrix( filt, R, b ) ) ); |
There was a problem hiding this comment.
Matrix( b, Grep ) may have the wrong BaseDomain since we have no control about BaseDomain( Grep ).
The dubious situations are when Grep is a list of lists, and as we have learned recently, the behaviour of Is8BitMatrixRep in this respect can be surprising.
|
|
||
| # ConvertToMatrixRepNC | ||
| gap> ConvertToMatrixRepNC([], 3); | ||
| gap> ConvertToMatrixRepNC([], 3); # What is this test supposed to show? |
There was a problem hiding this comment.
It was probably just added for "test coverage", and to perhaps to ensure there is no crash. Anyway, I think it would be acceptable to also remove it.
There was a problem hiding this comment.
The result 3 is irritating, since the first argument is in fact not in a compressed representation afterwards.
fingolfin
left a comment
fingolfin
left a comment
There was a problem hiding this comment.
Looks good to me. Perhaps the title should be adjusted?
ThomasBreuer
changed the title
Matrix callsConvertToMatrixRep, fix some Matrix calls
2 participants
Address the problem from #6389, for the moment without introducing
BaseDomainfor matrix groups.I regard this pull request as an alternative to #6395.
As stated in #6389, calling
Matrix( elm, Grep )is possible only ifGrepis a proper matrix object; ifGrepis a list of lists then it may have a too smallBaseDomain. Thus theMatrixcalls introduced in #6232 have to be replaced by calls where theConstructingFilterand theBaseDomainare given as arguments.