Speed up OnDigraphs for a digraph and a permutation (#267)

* Speed up OnDigraphs for a graph and a permutation

* Instead of checking if the permutation fixes the vertices, check
if it is the identity (which is much cheaper, although catches
less cases)

* Do not check explictly if the permutation maps the vertices
  of the graph to itself, do the mapping then check afterwards.

* Use SmallestMovedPoint optimization, move error checking to make Permuted safe.

* No commas or dots in error messages for OnDigraphs (directions from above)

* Roll back change

* Add OnDigraphsNC

* Fix lint

* Use OnDigraphsNC internally

* Try with identity perm check

---------

Co-authored-by: reiniscirpons <rc234@st-andrews.ac.uk>

Author: ChrisJefferson

gap/attr.gi

@@ -2744,10 +2744,10 @@ function(D)
   p    := Permutation(Transformation(topo), topo);
 
   C := DigraphMutableCopyIfImmutable(D);
-  OnDigraphs(C, p ^ -1);       # changes C in-place
-  DIGRAPH_TRANS_REDUCTION(C);  # changes C in-place
+  OnDigraphsNC(C, p ^ -1);       # changes C in-place
+  DIGRAPH_TRANS_REDUCTION(C);    # changes C in-place
   ClearDigraphEdgeLabels(C);
-  OnDigraphs(C, p);            # changes C in-place
+  OnDigraphsNC(C, p);            # changes C in-place
   if IsImmutableDigraph(D) then
     MakeImmutable(C);
     SetDigraphTransitiveReductionAttr(D, C);

gap/grahom.gi

@@ -819,7 +819,7 @@ function(L1, L2)
   in1, in2, mask, defined, not_in_image, FindNextAmong, Recurse;
 
   p := PermList(Reversed(DigraphWelshPowellOrder(L1)));
-  L1 := OnDigraphs(L1, p ^ -1);
+  L1 := OnDigraphsNC(L1, p ^ -1);
 
   # We compute the join/meet table to avoid having to do this twice if L1 or L2
   # is mutable

gap/isomorph.gi

@@ -214,7 +214,7 @@ function(D)
   if IsMultiDigraph(D) then
     return OnMultiDigraphs(D, BlissCanonicalLabelling(D));
   fi;
-  return OnDigraphs(D, BlissCanonicalLabelling(D));
+  return OnDigraphsNC(D, BlissCanonicalLabelling(D));
 end);
 
 InstallMethod(BlissCanonicalDigraph, "for a digraph and vertex coloring",
@@ -223,7 +223,7 @@ function(D, colors)
   if IsMultiDigraph(D) then
     return OnMultiDigraphs(D, BlissCanonicalLabelling(D, colors));
   fi;
-  return OnDigraphs(D, BlissCanonicalLabelling(D, colors));
+  return OnDigraphsNC(D, BlissCanonicalLabelling(D, colors));
 end);
 
 InstallMethodThatReturnsDigraph(NautyCanonicalDigraph, "for a digraph",
@@ -233,7 +233,7 @@ function(D)
     Info(InfoWarning, 1, "NautyTracesInterface is not available");
     return fail;
   fi;
-  return OnDigraphs(D, NautyCanonicalLabelling(D));
+  return OnDigraphsNC(D, NautyCanonicalLabelling(D));
 end);
 
 InstallMethod(NautyCanonicalDigraph, "for a digraph and vertex coloring",
@@ -243,7 +243,7 @@ function(D, colors)
     Info(InfoWarning, 1, "NautyTracesInterface is not available");
     return fail;
   fi;
-  return OnDigraphs(D, NautyCanonicalLabelling(D, colors));
+  return OnDigraphsNC(D, NautyCanonicalLabelling(D, colors));
 end);
 
 # Automorphism group
@@ -349,7 +349,7 @@ function(C, D)
   if IsMultiDigraph(C) then
     act := OnMultiDigraphs;
   else
-    act := OnDigraphs;
+    act := OnDigraphsNC;
   fi;
 
   if HasBlissCanonicalLabelling(C) and HasBlissCanonicalLabelling(D)
@@ -400,7 +400,7 @@ function(C, D, c1, c2)
   elif IsMultiDigraph(C) then
     act := OnMultiDigraphs;
   else
-    act := OnDigraphs;
+    act := OnDigraphsNC;
   fi;
 
   if DIGRAPHS_UsingBliss or IsMultiDigraph(C) then
@@ -489,7 +489,7 @@ function(C, D, c1, c2)
     return [label1[1] / label2[1], label1[2] / label2[2]];
   fi;
 
-  if OnDigraphs(C, label1) <> OnDigraphs(D, label2) then
+  if OnDigraphsNC(C, label1) <> OnDigraphsNC(D, label2) then
     return fail;
   fi;
   return label1 / label2;

gap/oper.gd

@@ -62,6 +62,8 @@ DeclareOperation("DIGRAPHS_GraphProduct", [IsDigraph, IsDigraph, IsFunction]);
 # 4. Actions . . .
 DeclareOperation("OnDigraphs", [IsDigraph, IsPerm]);
 DeclareOperation("OnDigraphs", [IsDigraph, IsTransformation]);
+DeclareOperation("OnDigraphsNC", [IsDigraph, IsPerm]);
+DeclareOperation("OnDigraphsNC", [IsDigraph, IsTransformation]);
 DeclareOperation("OnTuplesDigraphs", [IsDigraphCollection, IsPerm]);
 DeclareOperation("OnSetsDigraphs", [IsDigraphCollection, IsPerm]);
 DeclareOperation("OnMultiDigraphs", [IsDigraph, IsPermCollection]);

gap/oper.gi

@@ -993,15 +993,25 @@ end);
 # 4. Actions
 ###############################################################################
 
-InstallMethod(OnDigraphs, "for a mutable digraph by out-neighbours and a perm",
-[IsMutableDigraph and IsDigraphByOutNeighboursRep, IsPerm],
+InstallMethod(OnDigraphs, "for a digraph and a perm",
+[IsDigraph, IsPerm],
 function(D, p)
-  local out;
   if ForAll(DigraphVertices(D), i -> i ^ p = i) then
     return D;
   elif ForAny(DigraphVertices(D), i -> i ^ p > DigraphNrVertices(D)) then
     ErrorNoReturn("the 2nd argument <p> must be a permutation that permutes ",
-                  "the vertices of the digraph <D> that is the 1st argument,");
+                  "the vertices of the digraph <D> that is the 1st argument");
+  fi;
+  return OnDigraphsNC(D, p);
+end);
+
+InstallMethod(OnDigraphsNC,
+"for a mutable digraph by out-neighbours and a perm",
+[IsMutableDigraph and IsDigraphByOutNeighboursRep, IsPerm],
+function(D, p)
+  local out;
+  if p = () then
+    return D;
   fi;
   out := D!.OutNeighbours;
   out{DigraphVertices(D)} := Permuted(out, p);
@@ -1010,26 +1020,40 @@ function(D, p)
   return D;
 end);
 
-InstallMethod(OnDigraphs, "for a immutable digraph and a perm",
+InstallMethod(OnDigraphsNC, "for a immutable digraph and a perm",
 [IsImmutableDigraph, IsPerm],
 function(D, p)
-  if ForAll(DigraphVertices(D), i -> i ^ p = i) then
+  local out, permed;
+  if p = () then
     return D;
   fi;
-  return MakeImmutable(OnDigraphs(DigraphMutableCopy(D), p));
+  out := D!.OutNeighbours;
+  permed := Permuted(out, p);
+  Apply(permed, x -> OnTuples(x, p));
+  return DigraphNC(IsImmutableDigraph, permed);
 end);
 
 InstallMethod(OnDigraphs,
-"for a mutable digraph by out-neighbours and a transformation",
-[IsMutableDigraph and IsDigraphByOutNeighboursRep, IsTransformation],
+"for a digraph and a transformation",
+[IsDigraph, IsTransformation],
 function(D, t)
-  local old, new, v;
   if ForAll(DigraphVertices(D), i -> i ^ t = i) then
     return D;
   elif ForAny(DigraphVertices(D), i -> i ^ t > DigraphNrVertices(D)) then
     ErrorNoReturn("the 2nd argument <t> must be a transformation that ",
                   "maps every vertex of the digraph <D> that is the 1st ",
-                  "argument, to another vertex.");
+                  "argument, to another vertex");
+  fi;
+  return OnDigraphsNC(D, t);
+end);
+
+InstallMethod(OnDigraphsNC,
+"for a mutable digraph by out-neighbours and a transformation",
+[IsMutableDigraph and IsDigraphByOutNeighboursRep, IsTransformation],
+function(D, t)
+  local old, new, v;
+  if t = IdentityTransformation then
+    return D;
   fi;
   old := D!.OutNeighbours;
   new := List(DigraphVertices(D), x -> []);
@@ -1041,13 +1065,19 @@ function(D, t)
   return D;
 end);
 
-InstallMethod(OnDigraphs, "for a immutable digraph and a transformation",
+InstallMethod(OnDigraphsNC, "for a immutable digraph and a transformation",
 [IsImmutableDigraph, IsTransformation],
 function(D, t)
-  if ForAll(DigraphVertices(D), i -> i ^ t = i) then
+  local old, new, v;
+  if t = IdentityTransformation then
     return D;
   fi;
-  return MakeImmutable(OnDigraphs(DigraphMutableCopy(D), t));
+  old := D!.OutNeighbours;
+  new := List(DigraphVertices(D), x -> []);
+  for v in DigraphVertices(D) do
+    Append(new[v ^ t], OnTuples(old[v], t));
+  od;
+  return DigraphNC(IsImmutableDigraph, new);
 end);
 
 InstallMethod(OnTuplesDigraphs,
@@ -1060,7 +1090,7 @@ InstallMethod(OnSetsDigraphs,
 [IsDigraphCollection and IsHomogeneousList, IsPerm],
 function(S, p)
   if not IsSet(S) then
-    ErrorNoReturn("the first argument must be a set (a strictly sorted list),");
+    ErrorNoReturn("the first argument must be a set (a strictly sorted list)");
   fi;
   return Set(S, D -> OnDigraphs(DigraphMutableCopyIfMutable(D), p));
 end);
@@ -1081,7 +1111,7 @@ function(D, perms)
             i -> i ^ perms[2] > DigraphNrEdges(D)) then
     ErrorNoReturn("the 2nd entry of the 2nd argument <perms> must ",
                   "permute the edges of the digraph <D> that is the 1st ",
-                  "argument,");
+                  "argument");
   fi;
 
   return OnDigraphs(D, perms[1]);

tst/standard/oper.tst

@@ -98,7 +98,7 @@ gap> h := (1, 2, 3, 4);
 (1,2,3,4)
 gap> OnDigraphs(gr, h);
 Error, the 2nd argument <p> must be a permutation that permutes the vertices o\
-f the digraph <D> that is the 1st argument,
+f the digraph <D> that is the 1st argument
 gap> gr := Digraph([[1, 1, 1, 3, 5], [], [3, 2, 4, 5], [2, 5], [1, 2, 1]]);
 <immutable multidigraph with 5 vertices, 14 edges>
 gap> DigraphEdges(gr);
@@ -108,7 +108,7 @@ gap> p1 := (2, 4)(3, 6, 5);
 (2,4)(3,6,5)
 gap> OnDigraphs(gr, p1);
 Error, the 2nd argument <p> must be a permutation that permutes the vertices o\
-f the digraph <D> that is the 1st argument,
+f the digraph <D> that is the 1st argument
 gap> p2 := (1, 3, 4, 2);
 (1,3,4,2)
 gap> OnDigraphs(gr, p2);
@@ -125,7 +125,7 @@ gap> p1 := (1, 5, 4, 2, 3);
 (1,5,4,2,3)
 gap> OnDigraphs(gr, p1);
 Error, the 2nd argument <p> must be a permutation that permutes the vertices o\
-f the digraph <D> that is the 1st argument,
+f the digraph <D> that is the 1st argument
 gap> p2 := (1, 4)(2, 3);
 (1,4)(2,3)
 gap> OnDigraphs(gr, p2);
@@ -142,7 +142,7 @@ gap> OutNeighbours(gr);
 gap> t := Transformation([4, 2, 3, 4]);;
 gap> OnDigraphs(gr, t);
 Error, the 2nd argument <t> must be a transformation that maps every vertex of\
- the digraph <D> that is the 1st argument, to another vertex.
+ the digraph <D> that is the 1st argument, to another vertex
 gap> t := Transformation([1, 2, 1]);;
 gap> gr := OnDigraphs(gr, t);
 <immutable multidigraph with 3 vertices, 3 edges>
@@ -173,7 +173,7 @@ gap> D := [DigraphReverse(ChainDigraph(3)), ChainDigraph(3)];;
 gap> IsSet(D);
 false
 gap> OnSetsDigraphs(D, (1, 2));
-Error, the first argument must be a set (a strictly sorted list),
+Error, the first argument must be a set (a strictly sorted list)
 gap> D := Reversed(D);;
 gap> OnSetsDigraphs(D, (1, 3)) = D;
 true
@@ -209,7 +209,7 @@ gap> OnMultiDigraphs(gr1, [(1, 3)]);
 Error, the 2nd argument <perms> must be a pair of permutations,
 gap> OnMultiDigraphs(gr1, [(1, 3), (1, 7)]);
 Error, the 2nd entry of the 2nd argument <perms> must permute the edges of the\
- digraph <D> that is the 1st argument,
+ digraph <D> that is the 1st argument
 
 #  DomainForAction
 gap> DomainForAction(RandomDigraph(10), SymmetricGroup(10), OnDigraphs);