Implement a new matrix object using flat lists in row-major form #5121
Closed
wucas wants to merge 3 commits intogap-system:masterfrom
Closed
Implement a new matrix object using flat lists in row-major form #5121wucas wants to merge 3 commits intogap-system:masterfrom
wucas wants to merge 3 commits intogap-system:masterfrom
Conversation
wucas
added
kind: enhancement
| [ IsFlistMatrixRep ], | ||
| function( mat ) | ||
| local n; | ||
| if m![FLISTREP_NCPOS] <> mat![FLISTREP_NRPOS] then |
Contributor
There was a problem hiding this comment.
you probably mean mat![FLISTREP_NCPOS] <> mat![FLISTREP_NRPOS]
fingolfin
reviewed
Oct 20, 2022
| and HasBaseDomain and HasOneOfBaseDomain and HasZeroOfBaseDomain, | ||
| [] ); | ||
|
|
||
| # If we implement our object a a positional object we often have to access its |
Member
There was a problem hiding this comment.
Suggested change
| # 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 |
Comment on lines
+68
to
+74
| 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 ); No newline at end of file |
Member
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.
Suggested change
| 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; |
Member
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]}); |
Member
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
Member
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 |
Member
There was a problem hiding this comment.
Suggested change
| if m![FLISTREP_NCPOS] <> mat![FLISTREP_NRPOS] then | |
| if mat![FLISTREP_NCPOS] <> mat![FLISTREP_NRPOS] then |
fingolfin
reviewed
Oct 20, 2022
| return m![FLISTREP_NCPOS]; | ||
| end ); | ||
|
|
||
| InstallMethod( DimensionsMat, "for a flist matrix", |
Member
There was a problem hiding this comment.
You don't need to provide this, as the default method should be good enough
danielrademacher
pushed a commit
to danielrademacher/gap
that referenced
this pull request
Oct 20, 2022
fingolfin
changed the title
Member
|
Superseded by PR #6322 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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