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.

Sign up for GitHub

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

Jump to bottom

IsMatrixObj objects as elements of GAP's matrix groups #3093

Merged
merged 1 commit into from
Dec 6, 2018
Merged

IsMatrixObj objects as elements of GAP's matrix groups #3093

merged 1 commit into from
Dec 6, 2018

Conversation

ThomasBreuer
Copy link
Contributor

GAP's default mechanism for dealing with small matrix groups
is to compute an isomorphic permutation group
via the action on orbits of vectors.

With the changes from this pull request,
this mechanism works for groups of objects in IsMatrixObj
that are not plain lists of lists.
Of course this is just one step, more changes in this area will follow.
(A preliminary test case is contained in the GAPJulia package.)

Some changes are hopefully not controversial,
such as replacing Length calls by calls of NumberRows,
and replacing matrix entry access m[i][j] by m[i,j].

For other changes, there would be alternatives.

  • Instead of introducing a new attribute ListOfRowsOfMatrix,
    one could declare ListOp( <matobj> ) for <matobj> in IsMatrixObj
    (and not only for <matobj> in IsRowListMatrix).
    From the viewpoint that the use of ListOp for IsMatrixObj matrices
    is not recommended, according to lib/matobj2.gd,
    I prefer ListOfRowsOfMatrix.

  • In the case of DefaultScalarDomainOfMatrixList,
    one could argue that also lists of IsMatrixObj objects with different BaseDomain can make sense,
    but I think that showing an error message is more in the spirit of IsMatrixObj.

@fingolfin
Copy link
Member

Great, thanks for working on this!

I agree that we should not have ListOp for MatrixObj, instead ListOfRowsOfMatrix seems like a better idea -- although I would suggest to rename it to just RowsOfMatrix, or is there a good reason not to?

@fingolfin fingolfin requested a review from hulpke December 6, 2018 09:42
fingolfin
fingolfin reviewed Dec 6, 2018
View reviewed changes
lib/matrix.gi Outdated
return f;
return f;
fi;
Error( "all in <l> must either be lists of lists or have the same BaseDomain" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Error( "all in <l> must either be lists of lists or have the same BaseDomain" );
Error( "all elements in <l> must either be lists of lists or have the same BaseDomain" );

hulpke
hulpke approved these changes Dec 6, 2018
View reviewed changes
Copy link
Contributor

@hulpke hulpke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me.

@hulpke
Copy link
Contributor

hulpke commented Dec 6, 2018

Are the proposed changes in accessing a matrix
a) stable and available in 4.10?
b) recommended already for other matrix group code ?
If so, is there a list of these changes (as I should modify my own code accordingly).

Thanks!

Alexander

@fingolfin
Copy link
Member

Accessing matrix entries via mat[i,j] is in GAP 4.10 and documented, and often is faster than mat[i][j]. Also NumberRows / NumberColumns (resp. their aliases NrRows / NrCols) are in 4.10.

The overall MatrixObj code is not yet properly documented, this is something we badly must work on, and I hope we can make at least partial progress on it for GAP 4.11. This will then on the long run allow us to overcome all kinds of issues, and help improve performance, reduce memory usage, etc. etc.

@ThomasBreuer
IsMatrixObj interface for GAP's matrix groups
b5a7672 
GAP's default mechanism for dealing with small matrix groups
is to compute an isomorphic permutation group
via the action on orbits of vectors.

With these changes,
this mechanism works for groups of objects in `IsMatrixObj`
that are not plain lists of lists.
Of course this is just one step, more changes in this area will follow.
(A preliminary test case is contained in the `GAPJulia` package.)

Some changes are hopefully not controversial,
such as replacing `Length` calls by calls of `NumberRows`,
and replacing matrix entry access `m[i][j]` by `m[i,j]`.

For other changes, there would be alternatives.

- Instead of introducing a new attribute `RowsOfMatrix`,
  one could declare `ListOp( <matobj> )` for `<matobj>` in `IsMatrixObj`
  (and not only for `<matobj>` in `IsRowListMatrix`).
  From the viewpoint that the use of `ListOp` for `IsMatrixObj` matrices
  is not recommended, according to `lib/matobj2.gd`,
  I prefer `RowsOfMatrix`.

- In `DefaultScalarDomainOfMatrixList`, one could argue that also
  lists of `IsMatrixObj` objects with different `BaseDomain`
  can make sense, but I think that showing an error message is more
  in the spirit of `IsMatrixObj`.
@codecov
Copy link

codecov bot commented Dec 6, 2018 •
edited
Loading

Codecov Report

Merging #3093 into master will decrease coverage by <.01%.
The diff coverage is 77.96%.

@@            Coverage Diff             @@
##           master    #3093      +/-   ##
==========================================
- Coverage   83.56%   83.56%   -0.01%     
==========================================
  Files         686      686              
  Lines      336733   336756      +23     
==========================================
+ Hits       281380   281396      +16     
- Misses      55353    55360       +7
Impacted Files Coverage Δ
lib/matobj.gi 63.57% <100%> (+0.51%) ⬆️
lib/zmodnz.gi 97.9% <100%> (ø) ⬆️
lib/matobj2.gd 100% <100%> (ø) ⬆️
lib/matrix.gi 86.8% <72%> (-0.17%) ⬇️
lib/grpmat.gi 71.02% <76%> (ø) ⬆️

@ThomasBreuer
Copy link
Contributor Author

@fingolfin Thanks for your comments, I have now updated the pull request.
Concerning the name ListOfRowsOfMatrix, my idea was to emphasize that the result is a list
(which is not obvious for objects that arise in the context of IsMatrixObj);
but let us try RowsOfMatrix.

@fingolfin fingolfin merged commit 554cde7 into gap-system:master Dec 6, 2018
@markusbaumeister markusbaumeister added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Apr 15, 2019
@ThomasBreuer ThomasBreuer deleted the TB_some_fixes_for_MatrixObj branch July 8, 2019 08:53
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

@hulpke hulpke hulpke approved these changes

Assignees
No one assigned
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library
Projects
None yet
Milestone
GAP 4.11.0
Development

Successfully merging this pull request may close these issues.
6 participants
@ThomasBreuer @fingolfin @hulpke @olexandr-konovalov @DominikBernhardt @markusbaumeister