From 940ab22200f14afe7aebf02d6114ac8a8646aeb3 Mon Sep 17 00:00:00 2001
From: Tobias Boege <post@taboege.de>
Date: Fri, 17 Jan 2025 11:24:35 +0100
Subject: [PATCH] Graphs: fix isCyclic Digraph

This fixes the detection of cycles in directed graphs (see issue #3626) by
not relying on the numbers from DFS but by attempting a topological sort.
It succeeds if and only if the graph is acyclic. This reuses existing code
as much as possible.

Also added tests that document previous bugs which are now fixed.
---
 M2/Macaulay2/packages/Graphs.m2 | 49 ++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/M2/Macaulay2/packages/Graphs.m2 b/M2/Macaulay2/packages/Graphs.m2
index 0b6df68b3ed..501f85b8991 100644
--- a/M2/Macaulay2/packages/Graphs.m2
+++ b/M2/Macaulay2/packages/Graphs.m2
@@ -1154,16 +1154,7 @@ spectrum Graph := List => G -> sort toList eigenvalues (adjacencyMatrix G, Hermi
 
 
 
-topologicalSort = method(TypicalValue =>List)
-topologicalSort Digraph := List => D -> topologicalSort(D, "")
-topologicalSort (Digraph, String) := List => (D,s) -> (
-    if instance(D, Graph) or isCyclic D then error "Topological sorting is only defined for acyclic directed graphs.";
-    s = toLower s;
-    processor := if s == "random" then random
-        else if s == "min" then sort
-        else if s == "max" then rsort
-        else if s == "degree" then L -> last \ sort transpose {apply(L, v -> degree(D, v)), L}
-        else identity;
+attemptTopologicalSort = (D,processor) -> (
     S := processor sources D;
     L := {};
     v := null;
@@ -1172,7 +1163,23 @@ topologicalSort (Digraph, String) := List => (D,s) -> (
         L = L|{v};
         S = processor join(drop(S, 1), select(toList children (D, v), c -> isSubset(parents(D, c), L)));
         );
-    L
+    if #L == #(vertexSet D) then L else null
+    )
+
+topologicalSort = method(TypicalValue =>List)
+topologicalSort Digraph := List => D -> topologicalSort(D, "")
+topologicalSort (Digraph, String) := List => (D,s) -> (
+    if instance(D, Digraph) then (
+        s = toLower s;
+        processor := if s == "random" then random
+            else if s == "min" then sort
+            else if s == "max" then rsort
+            else if s == "degree" then L -> last \ sort transpose {apply(L, v -> degree(D, v)), L}
+            else identity;
+        L := attemptTopologicalSort(D, processor);
+        if L =!= null then return L;
+    );
+    error "Topological sorting is only defined for acyclic directed graphs.";
     )
 
 
@@ -1270,14 +1277,7 @@ isConnected Graph := Boolean => G -> numberOfComponents G <= 1
 
 isCyclic = method()
 isCyclic Graph := Boolean => G -> isConnected G and all(vertexSet G, v -> degree(G, v) == 2)
-isCyclic Digraph := Boolean => G -> (
-        D := depthFirstSearch G;
-        any(vertexSet G, u ->
-            any(toList children(G, u), v ->
-                (D#symbol discoveryTime)#v < (D#symbol discoveryTime)#u and (D#symbol finishingTime)#u < (D#symbol finishingTime)#v
-                )
-            )
-        )
+isCyclic Digraph := Boolean => G -> attemptTopologicalSort(G, identity) === null
 
 isEulerian = method()
 isEulerian Graph := Boolean => G -> all(apply(vertexSet G, v -> degree(G,v)), even) and isConnected G
@@ -5505,6 +5505,17 @@ TEST ///
    assert(topologicalSort D==={2,3,1})
 ///
 
+TEST ///
+-- check cycle detection in digraphs
+assert( isCyclic digraph({{1,2},{2,3},{3,1}}) === true )
+assert( isCyclic digraph({{1,2},{2,3},{3,4},{4,3}}) === true )
+assert( isCyclic digraph({{1,2},{2,3},{3,4},{2,4}}) === false )
+assert( isCyclic digraph({{1,3},{1,4},{2,4},{3,2},{4,3}}) === true )
+assert( isCyclic digraph({{1,3},{1,4},      {3,2},{4,3}}) === false )
+assert( isCyclic digraph({{1,2},{2,3},{3,4}}) === false )
+assert( isCyclic digraph({{1,2},{2,3},{3,4},{4,4}}) === true )
+assert( isCyclic digraph({{1,2},{2,3},{3,4},{5,5}}) === true )
+///
 
 TEST ///