Skip to content
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

feat(#124): full traverse #125

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

williamfzc
Copy link
Contributor

@williamfzc williamfzc commented May 16, 2023

Hi :)

This is an implementation (draft) for #124 .

  • add TopologicalEntries for searching entries
  • add FullTraverse for walking all the nodes

It already matches my need. However, as a library, there are still some issues that need to be discussed:

  • It does not include the semantics of BFS or DFS
  • it may visits some nodes multiple times

@dominikbraun dominikbraun self-requested a review May 17, 2023 12:19
@dominikbraun
Copy link
Owner

Hi! Thanks for the PR. I do have one question in mind: What is the advantage of/difference between FullTraverse and a regular loop over the predecessors? Like this:

predecessorMap, _ := g.PredecessorMap()

for vertex, _ := range predecessorMap {
    _ = BFS(g, vertex, visit)
}

@williamfzc
Copy link
Contributor Author

Hi! Thanks for the PR. I do have one question in mind: What is the advantage of/difference between FullTraverse and a regular loop over the predecessors? Like this:

predecessorMap, _ := g.PredecessorMap()

for vertex, _ := range predecessorMap {
    _ = BFS(g, vertex, visit)
}

This loop seems it will initiate BFS on all vertices in the graph.
But the FullTraverse will only initiate BFS on the entry vertices.

I am not pretty sure that which one is better/expected.

@jonbrandenburg
Copy link
Contributor

jonbrandenburg commented Jul 14, 2023

Hi! Thanks for the PR. I do have one question in mind: What is the advantage of/difference between FullTraverse and a regular loop over the predecessors? Like this:

predecessorMap, _ := g.PredecessorMap()

for vertex, _ := range predecessorMap {
    _ = BFS(g, vertex, visit)
}

This loop seems it will initiate BFS on all vertices in the graph. But the FullTraverse will only initiate BFS on the entry vertices.

I am not pretty sure that which one is better/expected.

I would argue that FullTraverse is the better solution, since it guarantees each vertex is only visited up to one time O(V). Whereas the sample code above will guarantee O(V^2) for a complete graph. That said however based on how it is currently written it appears FullTraverse will not visit every vertex if the graph has cycles. By that definition I wouldn't consider it a FullTraverse.

If the intention is to simply visit every vertex, then adding a Vertices function to the graph would likely be a better solution or a Visit function at the that took a visit function as a parameter and handled the traversal automatically, preferably without making a clone of the Vertices which is likely what a Vertices function would force the user to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants