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

Add basic operations for row/column reductions in matrix objects #4517

Merged
merged 24 commits into from
Jun 15, 2021

Conversation

wucas
Copy link
Contributor

@wucas wucas commented May 27, 2021

Description

Closes #3962

Text for release notes

Implements elementary matrix operation for MatrixObjects

Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

I have left some comments

lib/matobj.gi Outdated Show resolved Hide resolved
lib/matobj.gi Outdated Show resolved Hide resolved
lib/matobj.gi Outdated Show resolved Hide resolved
lib/matobj.gi Outdated Show resolved Hide resolved
lib/matobj.gi Outdated Show resolved Hide resolved
lib/matobjplist.gi Outdated Show resolved Hide resolved
lib/matobjplist.gi Outdated Show resolved Hide resolved
lib/matobjplist.gi Outdated Show resolved Hide resolved
lib/matobjplist.gi Outdated Show resolved Hide resolved
lib/matobjplist.gi Outdated Show resolved Hide resolved
lib/matobj.gi Outdated Show resolved Hide resolved
@ssiccha ssiccha added this to In progress in MatrixObj via automation May 27, 2021
lib/matobj.gi Outdated Show resolved Hide resolved
lib/matobj.gi Outdated
# Checks
if not( 0 < row and row <= NrRows(mat) ) then
ErrorNoReturn("the second argument <row> has to fulfill 0 < row <= NrRows(mat) ");
fi;
Copy link
Member

Choose a reason for hiding this comment

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

According to Codecov, this function is lacking tests

Copy link

Choose a reason for hiding this comment

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

So far, we have only written tests for SwapMatrixRows and SwapMatrixColumns. We plan to also write tests for all other functions that we added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tests for all functions have been added.

Copy link
Contributor

@danielrademacher danielrademacher May 29, 2021

Choose a reason for hiding this comment

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

It seems like GAP is always using the "IsRowListMatrix" functions. Is there a way to define a matrix object which is not a "RowListMatrix"?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. In the end, this is what IsPlistMatrixRep (or perhaps IsStrictPlistMatrixRep) should do; see PR #2973 . It shouldn't be hard to do that atop the mentioned PR.

lib/matobj.gi Outdated Show resolved Hide resolved
lib/matobj.gi Outdated Show resolved Hide resolved
lib/matobj2.gd Outdated Show resolved Hide resolved
lib/matobj.gi Outdated Show resolved Hide resolved
lib/matobj2.gd Outdated Show resolved Hide resolved
lib/matobjplist.gi Outdated Show resolved Hide resolved
lib/matobjplist.gi Outdated Show resolved Hide resolved
Copy link
Contributor

@danielrademacher danielrademacher left a comment

Choose a reason for hiding this comment

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

The commit e334988 produced an error. Therefore, the old code of the "SwapMatrixRows" function is used again.

temp := mat[row1];
mat[row1] := mat[row2];
mat[row2] := temp;
mat{[row1,row2]} := mat{[row2,row1]};

Copy link
Contributor

@danielrademacher danielrademacher Jun 2, 2021

Choose a reason for hiding this comment

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

This gives us an error:

Error, List Assignments: <rhss> must be a dense list (not a positional object) in
  mat{[ row1, row2 ]} := mat{[ row2, row1 ]}; at .../GAP/gap/lib/matobjplist.gi:1351 called from 
<function "SwapMatrixRows for a mutable IsRowListMatrix, one row number, second row number">( <arguments> )
 called from read-eval loop at *stdin*:6
type 'quit;' to quit to outer loop

Copy link
Contributor

Choose a reason for hiding this comment

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

This problem shows a bug in GAP, see issue #4533.

@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library labels Jun 5, 2021
@fingolfin fingolfin changed the title Basic operations for row/column reductions Add basic operations for row/column reductions in matrix objects Jun 5, 2021
@fingolfin fingolfin added the release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes label Jun 5, 2021
lib/matobj2.gd Outdated Show resolved Hide resolved
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

This PR is still marked as draft. What would you like to add before considering this ready for final review and merging?

lib/matrobjrowlist.gi Outdated Show resolved Hide resolved
lib/matrobjrowlist.gi Outdated Show resolved Hide resolved
@danielrademacher
Copy link
Contributor

danielrademacher commented Jun 9, 2021

At the moment some functions cannot be tested because we don't have a MatrixObj which is not a IsRowListMatrix. Everything else should be finished (A little bit disappointed because of the slow performance mentioned in #3962 ).

@ssiccha
Copy link
Contributor

ssiccha commented Jun 10, 2021

A little bit disappointed because of the slow performance mentioned in #3962

The slow performance is only for IsMatrix lists of lists, though. Of course it would be nice to also have a performant version for those matrices, but we can work on that later.

@danielrademacher danielrademacher marked this pull request as ready for review June 10, 2021 09:42
@fingolfin
Copy link
Member

PR #4557 may help! Once that's in, we could use it here (this actually motivated me to finally tackle InstallEarlyMethod)

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

These are a good start and should be merged; we can improve them later on, after all (better tests, performance tuning, using InstallEarlyMethod, etc.)

@fingolfin fingolfin merged commit 9cf95be into gap-system:master Jun 15, 2021
MatrixObj automation moved this from In progress to Done Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
MatrixObj
  
Done
Development

Successfully merging this pull request may close these issues.

MatrixObj: basic operations for row/column reduction
6 participants