-
Notifications
You must be signed in to change notification settings - Fork 69
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
Implement connectedComponents and isConnected #216
base: main
Are you sure you want to change the base?
Conversation
@Synthetica9 Many thanks for the PR! I'm currently travelling and have a few other PRs to review, but I'll try to come back to you soon! |
I did not had the opportunity to start working on this yet but since you presented your work I think the best thing is to use your code! Once I'm free from other academic tasks I can maybe we can go through this together :) |
Benchmarks ========== These benchmarks were done with the following program: ```haskell import Algebra.Graph (Graph) import Algebra.Graph.NonEmpty (components, vertexCount) import Algebra.Graph.Test.Arbitrary import Data.Foldable import qualified Data.Set as S import Test.QuickCheck import Test.QuickCheck.Gen import Test.QuickCheck.Random genSeed seed (MkGen g) = do let r = mkQCGen seed return (g r 30) main :: IO () main = do let gen = (resize 100 $ arbitrary) :: Gen (Graph Int) for_ [1..1000] $ \seed -> do g <- genSeed seed gen print (vertexCount <$> (S.toList $ components g)) ``` Ran on my own laptop. Not very scientific, but better than nothing, and the improvement is clear. The improvements mostly seem to be in the worst-case performance. When not using a fixed seed, times varied drastically. This does introduce a new dependency, but I think the speed improvement is worth it. 5 graphs of depth 1000 ---------------------- **New** 14.91user 0.04system 0:14.96elapsed 99%CPU (0avgtext+0avgdata 30688maxresident)k 0inputs+0outputs (0major+19991minor)pagefaults 0swaps **Old** 188.67user 0.44system 3:09.12elapsed 99%CPU (0avgtext+0avgdata 31592maxresident)k 0inputs+0outputs (0major+18148minor)pagefaults 0swaps 12x improvement 1000 graphs of depth 100 ------------------------ **New** 3.55user 0.02system 0:03.57elapsed 99%CPU (0avgtext+0avgdata 7408maxresident)k 0inputs+0outputs (0major+945minor)pagefaults 0swaps **Old** 98.67user 0.21system 1:38.91elapsed 99%CPU (0avgtext+0avgdata 7352maxresident)k 0inputs+0outputs (0major+942minor)pagefaults 0swaps 27x improvement
5e6635f
to
bee8624
Compare
because I didn't want to add your name and contact details without your consent. Please fix this | ||
by sending a PR, keeping the list alphabetical. | ||
|
||
Also see the autogenerated yet still possibly incomplete | ||
[list of contributors](https://github.com/snowleopard/alga/graphs/contributors). | ||
|
||
Thank you all for your help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These spaces are important for rendering, please undo whitespace changes in this file.
@@ -94,7 +94,8 @@ library | |||
base >= 4.7 && < 5, | |||
containers >= 0.5.5.1 && < 0.8, | |||
deepseq >= 1.3.0.1 && < 1.5, | |||
mtl >= 2.1 && < 2.3 | |||
mtl >= 2.1 && < 2.3, | |||
equivalence >= 0.3 && < 0.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid introducing new dependencies.
Could you list the functionality you use along with asymptotic complexity?
-- isConnected $ vertex x + vertex y == False | ||
-- @ | ||
isConnected :: Ord a => G.Graph a -> Bool | ||
isConnected g = case Set.toList $ components g of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you mean connected components in the sense of undirected graphs, right? If yes, this should be clarified and also moved to a more fitting module, e.g. Algebra.Graph.Undirected.Algorithm
.
@Synthetica9 It's been a while -- have you had a chance to look at the comments above? |
Based on #196.
I derived my own algorithm to get the connected components directly from the graph's structure. I'm not 100% on the complexity of it.
I'm also not 100% sure where these functions should go. They are intended for general graphs, but we can guarantee that every component isn't empty.
Also, there doesn't seem to be a place to put small tools that might be useful elsewhere, am I overlooking this?I was!src/Algebra/Graph/Internal.hs
cc @bolt12
(Did you get to this already? I couldn't find you work anywhere)TODO:
Set.disjoint
turns out to be fairly new. Backport?