Added algorithms for finding strong components and cycles. #65

gablin · 2017-05-03T14:15:44Z

No description provided.

ivan-m

Please also add some tests for these functions.

ivan-m · 2017-05-04T00:37:43Z

Data/Graph/Inductive/Query/Cycles.hs

+   Module      : Data.Graph.Inductive.Query.Cycles
+   Description : Algorithms for finding all cycles.
+   Copyright   : (c) Gabriel Hjort Blindell 2017
+   License     : 2-Clause BSD


FGL is licensed under 3-Clause BSD; in interests of using the same license for all, would you mind changing this?

(And some code from here is from Graphalyze, which isn't under your Copyright.)

Addressed as part of the reply to your comment below.

I decided to include a header, with the BSD3 license and copyright held by both me and you (due to copied functions from Graphalize package).

ivan-m · 2017-05-04T00:38:49Z

Data/Graph/Inductive/Query/Cycles.hs

+   Description : Algorithms for finding all cycles.
+   Copyright   : (c) Gabriel Hjort Blindell 2017
+   License     : 2-Clause BSD
+   Maintainer  : [email protected]


Will you be maintaining this module?

(Actually, if you look at all the current FGL modules they tend not to bother with the header block at all, so you could just remove the entire thing.)

Ah, I didn't read the header carefully. =)

I've removed the header entirely.

Added header, but removed maintainer information.

ivan-m · 2017-05-04T00:39:36Z

Data/Graph/Inductive/Query/Cycles.hs

+
+import Data.Graph.Inductive.Graph
+
+import Data.List


Please put import blocks on the same line, e.g.

import Data.List ((\\), delete, tails)

ivan-m · 2017-05-04T00:44:26Z

Data/Graph/Inductive/Query/Cycles.hs

+        -- index, and its low link).
+      , sccGraph :: g a b
+        -- ^ The input graph.
+      }


Why no instances? At least Eq, Show and Read plaese.

Show, Read, and Eq added.

ivan-m · 2017-05-04T00:45:37Z

Data/Graph/Inductive/Query/Cycles.hs

+  = SCCState
+      { sccComponents :: [g a b]
+        -- ^ The components found so far.
+      , sccCurrentIndex :: Int


This (and others) should probably be banged.

I've not used bangs before, so I'm not sure of whether they apply here or not. I'll try looking into it.

ivan-m · 2017-05-04T00:48:04Z

Data/Graph/Inductive/Query/Cycles.hs

+                , sccNodeInfo = M.insert n (True, i, i) (sccNodeInfo st0)
+                }
+      g = sccGraph st1
+      st2 = foldr ( \m st ->


This is large enough that I think a new function - even if it's in a where block - is warranted for documentation, etc.

That way, you can also use guards rather than nested if/else.

ivan-m · 2017-05-04T00:49:07Z

Data/Graph/Inductive/Query/Cycles.hs

+                }
+      g = sccGraph st1
+      st2 = foldr ( \m st ->
+                    let st_ni = sccNodeInfo st


Any reason for snake_case here?

Did some refactoring, and the only use of snake_case here is to distinguish the low link of the n node from the low link of the m node.

ivan-m · 2017-05-04T00:50:58Z

Data/Graph/Inductive/Query/Cycles.hs

+        -- ^ The current index.
+      , sccStack :: [Node]
+        -- ^ The node stack.
+      , sccNodeInfo :: M.Map Node (Bool, Int, Int)


I think a specific data structure with record labels would work better than a triple here; it would not only provide documentation as to what each field means, but you can then use them as selectors rather than pattern matching on the entire triple to get values out.

ivan-m · 2017-05-04T00:52:25Z

Data/Graph/Inductive/Query/Cycles.hs

+  ( fromJust )
+import Control.Monad
+  ( ap )
+import qualified Data.Map as M


Why not an IntMap?

Ah, wasn't aware of that module. =)
Fixed.

ivan-m · 2017-05-04T00:54:43Z

Data/Graph/Inductive/Query/Cycles.hs

+        -- ^ The components of the input graph.
+      , cisGraph :: g a b
+        -- ^ The input graph.
+      }


Instances please

Show, Read, and Eq added.

Still needs bangs: ![[Node]], !(M.IntMap Bool), etc.

Bang patterns added.

…yclesIn' to 'cycles'.

… class type.

… duplicate nodes (hence the node indicating the start and end of a cycle appears only once).

gablin · 2017-05-04T13:42:47Z

I've addressed all requests now, except for the one regarding bang patterns.

JeffreyBenjaminBrown · 2017-09-28T22:49:19Z

Hi! I would like to detect cycles. Is this held up for lack of tests? I could write some ...

gablin · 2017-09-29T13:41:17Z

Don't know. I thought the tests as part of the change request were sufficient, but maybe it's due to the fact that the bang patterns are missing, but I don't know how to fix that.

@ivan-m Is there anything else I need to do in order to make the pull request go through?

ivan-m · 2017-09-29T15:11:28Z

Data/Graph/Inductive/Query/Cycles.hs

+      c = fromJust $ cisCurrentComp st1
+      n_suc = suc c n
+      (st2, f) =
+        foldr ( \m (st, f') ->


This is big enough that it should be split into a separate function.

Now broken into several, smaller functions.

ivan-m · 2017-09-29T15:12:34Z

test/Data/Graph/Inductive/Query/Properties.hs

+test_strongComponentsOf :: (Graph gr) => Proxy (gr a b) -> gr a b -> Bool
+test_strongComponentsOf _ g =
+  sort (concatMap nodes (strongComponentsOf g)) == sort (nodes g)
+


Are you able to add a property as to the behaviour of the results of strongComponentsOf (e.g. disjoint)?

Test case has been expanded with additional property checks.

ivan-m · 2017-09-29T15:13:54Z

Data/Graph/Inductive/Query/Cycles.hs

+cycles :: (Graph g) => g a b -> [[LNode a]]
+cycles g = map (addLabels g) (cycles' g)
+
+-- | Same as 'cycles' but for unlabeled graphs.


Not for unlabelled graphs, just doesn't return the labels.

…between any two nodes belonging to separate components.

gablin · 2017-10-03T08:24:46Z

@ivan-m Requested changes have now been addressed.

ivan-m · 2017-10-03T08:28:27Z

Data/Graph/Inductive/Query/Cycles.hs

+       then let (st1, f1) = cCircuits n st0
+            in (st1, f0 || f1)
+       else (st0, f0)
+


Could have been in a where clause rather than top level.

Maybe consider guards rather than nested if/then/else?

The fromJust can also potentially throw an error. Using PatternGuards can make that neater.

cCircuitsVisit n (st0, f0) | maybe False (n==) (cisS st0) = ... | fromMaybe False (M.lookup n (cisBlocked st0)) = ... | otherwise = ...

(Or else use case (cisS st0, M.lookup n (cisBlocked st0)) of (Just n', _) | n == n' -> ...; (_, Just True) ...).

Same goes with the other functions.

Now using guards and removed need for fromJust.

…function parameter instead (this removes the need for using Maybe, which could fail).

… value as function parameter instead (this removes the need for using Maybe, which could fail).

ivan-m · 2017-10-03T09:05:33Z

Data/Graph/Inductive/Query/Cycles.hs

@@ -100,45 +96,41 @@ cyclesFor n st0 =
               filter (\c -> n `gelem` c) $
               cisComponents st0
  in if noNodes n_comp > 1


Note that noNodes is O(|V|) for PatriciaTree (but O(1) for Tree); it would probably be better to do something like not . null . drop 1 . nodes $ n_comp which should be O(1).

Ah, didn't know that. Fixed.

…ead of potentially O(n).

georgefst · 2020-12-15T18:04:20Z

Any reason this never got merged? I could really do with it.

(I came here trying to work out whether the outputs of scc are guaranteed to be paths (EDIT: this can't be true in general - I'm not sure what I was getting at here), but this PR would solve my immediate problems and many more)

Added algorithms for finding strong components and cycles.

5996a24

ivan-m requested changes May 4, 2017

View reviewed changes

Gabriel Hjort Blindell added 18 commits May 4, 2017 10:56

Removed header.

ca867a9

Added comment about functions copied from other packages.

205af43

Put import blocks on the same line, according to coding convention.

b9019fe

Replaced use of Map with IntMap.

6ce6e5d

Replaced use of tuple with a new data type.

73707fe

Added instances to data types.

6d51e8b

Added wiki link to Tarjan's SCC algorithm.

003c96c

Broken out large anonymous functions into named functions.

c8649fa

Put short if-then-else statement on the same line.

ab5064a

Code refactoring.

514151b

Broken out SCC parts from Cycles into its own module. Also renamed 'c…

f2e043e

…yclesIn' to 'cycles'.

Added header.

8e4102c

Cycles and SCC modules no longer require the graphs to be of DynGraph…

874da10

… class type.

Added tests for SCC module.

73694ef

Added tests for the Cycles module.

a8b439b

Improved documentation. Lists returned from cycles no longer contains…

ed7df27

… duplicate nodes (hence the node indicating the start and end of a cycle appears only once).

Fixed bug.

112309d

Fixed test case.

0dc411a

ivan-m reviewed Sep 29, 2017

View reviewed changes

Gabriel Hjort Blindell added 4 commits October 3, 2017 09:34

Broken down cCircuits into several smaller functions.

bd818d2

Fixed erronous function descriptions of cycles' and uniqueCycles'.

99948d3

Added missing bang patterns to CyclesInState.

766a4f2

Added missing bang patterns to SCCState.

3ce62ca

Gabriel Hjort Blindell added 2 commits October 3, 2017 10:12

Fixed typo.

c06f25c

Expanded test_strongComponentsOf to check that there exists no cycle …

a60460e

…between any two nodes belonging to separate components.

ivan-m reviewed Oct 3, 2017

View reviewed changes

Gabriel Hjort Blindell added 3 commits October 3, 2017 10:38

Rewrote cCircuitsVisit using guards.

0fd7130

Removed cisS field from CyclesInState, and now passing that value as …

d3e2a0f

…function parameter instead (this removes the need for using Maybe, which could fail).

Removed cisCurrentComp field from CyclesInState, and now passing that…

77e2d30

… value as function parameter instead (this removes the need for using Maybe, which could fail).

ivan-m reviewed Oct 3, 2017

View reviewed changes

Replaced use of noNodes with another method that only takes O(1) inst…

4dd06b7

…ead of potentially O(n).

ivan-m force-pushed the master branch from 878aec0 to d588cef Compare January 28, 2020 13:33

Added algorithms for finding strong components and cycles. #65

Are you sure you want to change the base?

Added algorithms for finding strong components and cycles. #65

Conversation

gablin commented May 3, 2017

ivan-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gablin commented May 4, 2017

JeffreyBenjaminBrown commented Sep 28, 2017

gablin commented Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gablin commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgefst commented Dec 15, 2020 • edited Loading

gablin commented Sep 29, 2017 •

edited

Loading

georgefst commented Dec 15, 2020 •

edited

Loading