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

Speed up OnDigraphs for a digraph and a permutation #267

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −6
Open

Speed up OnDigraphs for a digraph and a permutation #267

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions gap/oper.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,15 +665,19 @@ end);
InstallMethod(OnDigraphs, "for a mutable digraph by out-neighbours and a perm",
[IsMutableDigraph and IsDigraphByOutNeighboursRep, IsPerm],
function(D, p)
local out;
if ForAll(DigraphVertices(D), i -> i ^ p = i) then
local out, permed;
if p = () then
Copy link
Collaborator

@wilfwilson wilfwilson Oct 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In one sense this is worse: previously we were testing whether p acts as the identity on DigraphVertices(D), which can be the case even if p is not the identity permutation. But I don't mind too much about losing that case.

Is p = () optimised somehow? Do all permutations inherently know whether or not they are the identity, for instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

P=() isn't specifically optimised, but it will almost always be very fast, espically if (as is common) the permutation moves the largest point it is defined on, as that is checked first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to say, we could use SmallestMovedPoint is we really care about saving this optimisation. That is still much faster than the old code, and would keep the same answer.

return D;
elif ForAny(DigraphVertices(D), i -> i ^ p > DigraphNrVertices(D)) then
fi;

out := D!.OutNeighbours;
permed := Permuted(out, p);
if Length(permed) > DigraphNrVertices(D) then
Comment on lines +674 to +675
Copy link
Collaborator

Choose a reason for hiding this comment

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

If p is invalid, Permuted simply won't work properly (I think) and cause its own Error to happen, which on the one hand is good because it means that the method prevents something improper happen, but it does mean that we lose the ability to give a proper error message.

ErrorNoReturn("the 2nd argument <p> must be a permutation that permutes ",
"of the digraph <D> that is the 1st argument,");
fi;
out := D!.OutNeighbours;
out{DigraphVertices(D)} := Permuted(out, p);

out{DigraphVertices(D)} := permed;
Apply(out, x -> OnTuples(x, p));
ClearDigraphEdgeLabels(D);
return D;
Expand All @@ -682,7 +686,7 @@ end);
InstallMethod(OnDigraphs, "for a immutable digraph and a perm",
[IsImmutableDigraph, IsPerm],
function(D, p)
if ForAll(DigraphVertices(D), i -> i ^ p = i) then
if p = () then
return D;
fi;
return MakeImmutable(OnDigraphs(DigraphMutableCopy(D), p));
Expand Down