-
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
Adding BFS support to AdjacencyMaps. #185
base: main
Are you sure you want to change the base?
Conversation
…alysis is missing.
I've started implementing the missing functions. By running For example, The type signature indicates the option (a) is the correct one, but (b) splits the different trees generated, which would require |
…desired output is still ambigous.
Yes. The first element of the list contains the roots that are present in the graph, the second element contains all vertices reachable from roots in 1 step, etc.
Neither a) not b) matches the desired output, which in this case should be just Another example with your graph: |
|
||
-- | Compute the /breadth-first search/ AdjacencyMap of a graph that corresponds to | ||
-- searching from a single vertex of the graph. | ||
-- This is just for internal use. Might move it to `*.Internal` then? |
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.
No need to create an internal module, simply don't export internal functions.
There was an error with my example. |
… rewrote bfsForest to reutilize code and make it cleaner.
@TheChouzanOne Thanks for adding complexity bounds. O(n^2 * log(n)) sounds too slow though! Can we get closer to linear O(n+m) or at least O((n+m)*log(n))? |
I've just realized that my rough estimate was completely wrong and the real bound was O (n^2). Still, pretty slow. I've changed the implementation of That log (v) comes from the fact that Also, I added a new internal function to achieve less running time, which I've documented. I've been pretty busy and haven't been able to update my blog, but plan to do so with these last updates once I have time. Anyways, what do you think about the current state of the code? |
Wait, I've just realized something and am confused. I am not sure if I am tired (it's 3 a.m.), but isn't O (v + e * log(v) ) not much of an improvement? Given a full graph, e=(v * (v-1)) /2 = O(v^2), so time complexity would be O (v^2 * log(v) ), which is worse than expected. Am I correct or tired? |
@TheChouzanOne This is a big improvement, because graphs are rarely fully connected! In the most typical case we have something like m = O(n). Thanks for the revision, I'll take a look soon. |
P.S.: I'm using n = |V| and m = |E|, as in the Alga documentation. |
I've changed documentation notation to n and m for vertices and edges, respectively. |
@TheChouzanOne Thanks for the fixes! At the moment the implementation looks unnecessarily complex to me. I don't understand why we need auxiliary functions like BFS is a very simple algorithm conceptually: you traverse all vertices starting with a root, marking each vertex with its parent. This information should be sufficient for extracting a tree for each root. Furthermore, each example should have an associated test in the testsuite. You can have a look at how each example is tested by looking at the implementation of DFS. |
I agree that implementation looks too complex. The reason I decided to use auxiliary functions like the one you mention was because I thought it owuld be easier to build an I will do some research on how this problem is normally solved with functional programming and rewrite the algorithms in a simpler manner. This might take some time given that I have many end of semester school projects at the moment.
Sounds good. I'll look into it. Does this have something to do with Travis CI build failing? I don't really have much experience with continous integration software. |
@TheChouzanOne No problem, take your time. I'll ping you if this becomes more urgent.
No, I don't think the testsuite is related to the CI failures. I've restarted Travis -- hopefully it will complete successully this time. |
I've added a bfsForest algorithm that makes use of "helper" functions for a total of 4 new functions. These are located in AdjacencyMap/Algorithm.hs, however, I believe that the 3 helped functions might be rellocated to keep Algorithm.hs clean.
Complexity analysis is still missing.
Also, the next functions need to be implemented.
A link to posts regarding the development of this PR can be found here.