Implement a new matrix object using flat lists in row-major form #5121
Conversation
wucas
added
kind: enhancement
| [ IsFlistMatrixRep ], | ||
| function( mat ) | ||
| local n; | ||
| if m![FLISTREP_NCPOS] <> mat![FLISTREP_NRPOS] then |
There was a problem hiding this comment.
you probably mean mat![FLISTREP_NCPOS] <> mat![FLISTREP_NRPOS]
| and HasBaseDomain and HasOneOfBaseDomain and HasZeroOfBaseDomain, | ||
| [] ); | ||
|
|
||
| # If we implement our object a a positional object we often have to access its |
There was a problem hiding this comment.
| # If we implement our object a a positional object we often have to access its | |
| # If we implement our object as a positional object we often have to access its |
| BindGlobal( "FLISTREP_BDPOS", 1 ); | ||
| # Position in the positional object of the number of rows | ||
| BindGlobal( "FLISTREP_NRPOS", 2 ); | ||
| # Position in the positional object of the number of columns | ||
| BindGlobal( "FLISTREP_NCPOS", 3 ); | ||
| # Position in the positional object of the list of entries | ||
| BindGlobal( "FLISTREP_ELSPOS", 4 ); |
There was a problem hiding this comment.
Make sure to end files with a newline. Also, we can use BindConstant here for a tiny further speed-up.
| BindGlobal( "FLISTREP_BDPOS", 1 ); | |
| # Position in the positional object of the number of rows | |
| BindGlobal( "FLISTREP_NRPOS", 2 ); | |
| # Position in the positional object of the number of columns | |
| BindGlobal( "FLISTREP_NCPOS", 3 ); | |
| # Position in the positional object of the list of entries | |
| BindGlobal( "FLISTREP_ELSPOS", 4 ); | |
| BindConstant( "FLISTREP_BDPOS", 1 ); | |
| # Position in the positional object of the number of rows | |
| BindConstant( "FLISTREP_NRPOS", 2 ); | |
| # Position in the positional object of the number of columns | |
| BindConstant( "FLISTREP_NCPOS", 3 ); | |
| # Position in the positional object of the list of entries | |
| BindConstant( "FLISTREP_ELSPOS", 4 ); | |
| Error( "NewMatrix: Length of list must be a multiple of ncols." ); | ||
| fi; | ||
| list := list_in; | ||
| fi; |
There was a problem hiding this comment.
There are a lot of "this code is not covered by a test" comments by the Codecov bot here. I guess a nice task for this week would be to work on exactly such tests, in a way that makes them re-usable (we discussed this before, so I am guessing that was part of your intention anyway). Great :-)
| local index_start, index_end; | ||
| index_start := (row-1)*mat![FLISTREP_NCPOS] + 1; | ||
| index_end := index_start + mat![FLISTREP_NCPOS]; | ||
| return NewVector(IsPlistVectorRep,mat![FLISTREP_BDPOS],mat![FLISTREP_ELSPOS]{[index_start..index_end]}); |
There was a problem hiding this comment.
I don't think this correct. In the sense that doing mat[i][j] := 0 won't work correctly. Will be discussed in my talk
There was a problem hiding this comment.
Suggestion: just return an immutable vector, then at least the above will raise an error.
Also see comments on PR #5130
| [ IsFlistMatrixRep ], | ||
| function( mat ) | ||
| local n; | ||
| if m![FLISTREP_NCPOS] <> mat![FLISTREP_NRPOS] then |
There was a problem hiding this comment.
| if m![FLISTREP_NCPOS] <> mat![FLISTREP_NRPOS] then | |
| if mat![FLISTREP_NCPOS] <> mat![FLISTREP_NRPOS] then |
| return m![FLISTREP_NCPOS]; | ||
| end ); | ||
|
|
||
| InstallMethod( DimensionsMat, "for a flist matrix", |
There was a problem hiding this comment.
You don't need to provide this, as the default method should be good enough
fingolfin
changed the title
This PR provides a work in progress of a a new matrix object representing matrices as flat lists in row-major form. It is intended to guide participants of the GAP Days in their work on other matrix objects.
Text for release notes
none