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

list assignment for matrix objects #4533

Open
ThomasBreuer opened this issue Jun 2, 2021 · 3 comments
Open

list assignment for matrix objects #4533

ThomasBreuer opened this issue Jun 2, 2021 · 3 comments

Comments

@ThomasBreuer
Copy link
Contributor

The following happens in GAP 4.11.1 as well as in the master branch.

gap> m:= NewZeroMatrix( IsPlistMatrixRep, Integers, 2, 3 );
<2x3-matrix over Integers>
gap> m{[1,2]}:= m{[2,1]};
Error, List Assignments: <rhss> must be a dense list (not a positional object)
[...]

The point is that the operation {}:= (ASSS_LIST) calls AsssListCheck, and this calls RequireDenseList for the third argument (the right hand side of the assignment). However, the matrix objects in question aren't lists, in particular they are not dense lists.
We want {}:= for manipulation matrix objects that aren't lists, thus the checks must be changed.

@fingolfin
Copy link
Member

Really? I thought the agreement was the opposite, i.e., that {} is only for lists. After all, it can not be efficiently implemented for general matrix objects...

See #4356 for proposed syntax extension that might make sense for matrix objects.

@ThomasBreuer
Copy link
Contributor Author

O.k., it is more complicated.

The problem in #4517 is about IsRowListMatrix, and currently {}:= is defined for this kind of matrix objects.
Thus the question is what we want: forbid {}:= for IsRowListMatrix (and explain this, analogous to the situation with IsVectorObj, in the section about element access) or change the kernel code that expects dense lists on the right hand side.

@fingolfin
Copy link
Member

Discussed this with Thomas today. One thing I wasn't aware is that IsRowListMatrix does not imply IsList (this is useful in so far as we may not want to require IsRowListMatrix to really implement the full API for a list type (whatever that means exactly).

But the GAP kernel currently has some checks in the code implementing {} resp. {}:= which checks that the right hand side is a dense list. We check this in a bunch of places in the kernel, both with assertions (e.g. in ASSS_LIST) and explicit checks like CheckIsDenseList("List Assignments", "rhss", objs); in AsssListDefault. We could relax those, but then we also need to go through several kernel functions related to these operations, and carefully change ELMW_LIST invocations to ELM_LIST, to deal with "holes" in the lists....

A maybe easier approach would be change the checks to test for "IsDenseList OR IsRowListMatrix" (here I am assuming that any matrix with IsRowListMatrix(matrix) = true is guaranteed to behave like a dense list in a few key aspects; e.g. matrix[i] is bound for all i in [1 .. Length(matrix)].

ThomasBreuer added a commit to ThomasBreuer/gap that referenced this issue Jun 11, 2021
The documentation of the relevant operations for objects in `IsRowListMatrix`
(list access, list assignment, `IsBound[]`, ...) implies that such objects are
"dense" (although they need not be lists),
and this follows also from the fact that such an object `M` is in `IsMatrixObj`
and hence admits entry access `M[i,j]`,
for `i`and `j` up to `NumberRows( M )` and `NumberColumns( M )`, respectively.
However, it is better to say this explicitly (see also gap-system#4533).
fingolfin pushed a commit that referenced this issue Jun 11, 2021
The documentation of the relevant operations for objects in `IsRowListMatrix`
(list access, list assignment, `IsBound[]`, ...) implies that such objects are
"dense" (although they need not be lists),
and this follows also from the fact that such an object `M` is in `IsMatrixObj`
and hence admits entry access `M[i,j]`,
for `i`and `j` up to `NumberRows( M )` and `NumberColumns( M )`, respectively.
However, it is better to say this explicitly (see also #4533).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: No status
Development

No branches or pull requests

2 participants