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 much faster IsOne method for matrices #5342

Merged

Conversation

fingolfin
Copy link
Member

Also add similar IsOne and IsZero methods for rowlist matobj

Resolves #5341 by @hulpke

Example (before these took veery long... I didn't time it, but let's say closer to minutes than to milliseconds):

gap> map:=RegularActionHomomorphism(MathieuGroup(12));;
gap> p:=Image(map);
<permutation group of size 95040 with 3 generators>
gap>
gap> m:=PermutationMat(p.1,95040,GF(2));;
gap> IsOne(m); time;
false
13
gap> ConvertToMatrixRep(m);
2
gap> IsOne(m); time;
false
0
gap>
gap> m2:=PermutationMat(One(p),95040,GF(2));;
gap> IsOne(m2); time;
true
81
gap> ConvertToMatrixRep(m2);
2
gap> IsOne(m2); time;
true
70

Also add similar IsOne and IsZero methods for rowlist matobj
@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: performance bugs or enhancements related to performance (improvements or regressions) labels Jan 26, 2023

ncols:= NrCols( mat );
for row in mat do
if PositionNonZero( row ) <= ncols then
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to suggest that this might be clearer if it was:

Suggested change
if PositionNonZero( row ) <= ncols then
if PositionNonZero( row ) <> fail then

only to discover that PositionNonZero returns Length(row) + 1 if no non-zero is located. Is this intentional, it seems to be at odds with Position.

Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation says that PositionNonZero is a special case of PositionNot, and PositionNot treats unbound entries as "not the element which shall be avoided".

if PositionNonZero( row ) <> i or not IsOne( row[i] ) then
return false;
fi;
if PositionNonZero( row, i ) <= ncols then
Copy link
Contributor

@james-d-mitchell james-d-mitchell Jan 26, 2023

Choose a reason for hiding this comment

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

Think you might have copy pasted lines 1408 to 1410 from IsZero here, this can't be right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry my mistake.

Comment on lines +1196 to +1198
if PositionNonZero( row, i ) <= ncols then
return false;
fi;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if PositionNonZero( row, i ) <= ncols then
return false;
fi;

Copy link
Contributor

@james-d-mitchell james-d-mitchell Jan 26, 2023

Choose a reason for hiding this comment

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

Same copy paste error as above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I'm mistaken, this is correct, thanks to @ThomasBreuer for pointing this out.

Copy link
Contributor

@james-d-mitchell james-d-mitchell left a comment

Choose a reason for hiding this comment

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

Looks good apart from the copy paste errors indicated. with no changes!

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.

Good.

(I will change the available IsOne method in matobjplist.gi accordingly, in an update of my pull request #5340.)

ThomasBreuer added a commit to ThomasBreuer/gap that referenced this pull request Jan 26, 2023
as in pull request gap-system#5342 for other `IsOne` methods
@ThomasBreuer ThomasBreuer merged commit fdf1f29 into gap-system:master Jan 27, 2023
@fingolfin fingolfin deleted the mh/IsOne-for-Matrix-speedup branch January 27, 2023 08:48
fingolfin pushed a commit that referenced this pull request Feb 2, 2023
as in pull request #5342 for other `IsOne` methods
@fingolfin fingolfin changed the title Add faster IsOne method for matrices Add much faster IsOne method for matrices Jan 22, 2024
@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 Jan 22, 2024
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: performance bugs or enhancements related to performance (improvements or regressions)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance regression for matrices.
3 participants