Miscellaneous changes to the groups code #1351
Merged
Conversation
This file contains 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
- add some type annotations - improve some docstrings - replace some low-level access by use of accessor functions - add `ncols(::MatrixGroupElem)` to complement `nrows(::MatrixGroupElem)` (they will always return the same value, but it seems natural to offer both, and will potentially prevent some confusion for some poor sod down the line)
|
|
||
| Return the number of columns of the underlying matrix of `x`. | ||
| """ | ||
| ncols(x::MatrixGroupElem) = ncols(matrix(x)) |
There was a problem hiding this comment.
Hmm.
My understanding of MatrixGroupElem is as follows:
The element x stores either an Oscar matrix in x.elm or a GAP matrix in x.X or both.
Thus matrix(x) may force the computation of an Oscar matrix from x.X.
(Note that x.elm calls a suitable getproperty method.)
Perhaps it would be good to get rid of the getproperty method which somehow hides the fact that the call means more than just a field access, and to call matrix(x) in all places where one really needs the Oscar matrix.
Then x.elm calls should be restricted to those situations where one is sure that the field is already set.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
ncols(::MatrixGroupElem)to complementnrows(::MatrixGroupElem)(they will always return the same value, but it seems natural to offer
both, and will potentially prevent some confusion for some poor sod
down the line)