Skip to content

feat(#82): List all path between vertices#137

Merged
dominikbraun merged 3 commits intodominikbraun:mainfrom
tzq0301:all-paths-between-vertices
Jul 4, 2023
Merged

feat(#82): List all path between vertices#137
dominikbraun merged 3 commits intodominikbraun:mainfrom
tzq0301:all-paths-between-vertices

Conversation

@tzq0301
Copy link
Copy Markdown
Contributor

@tzq0301 tzq0301 commented Jun 13, 2023

The algorithm uses 2 stack and a loop to find out all paths between 2 vertices. It could be applied to both directed-graph and undirected-graph.

@tzq0301 tzq0301 force-pushed the all-paths-between-vertices branch 4 times, most recently from 5b149e4 to da075bf Compare June 13, 2023 06:53
@dominikbraun dominikbraun self-requested a review June 13, 2023 11:04
@tzq0301 tzq0301 force-pushed the all-paths-between-vertices branch from 179dfae to 5d136fa Compare June 15, 2023 02:36
@tzq0301 tzq0301 force-pushed the all-paths-between-vertices branch from 5d136fa to b760320 Compare June 16, 2023 08:16
Copy link
Copy Markdown
Owner

@dominikbraun dominikbraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for implementing this!

This is a very interesting algorithm. Do you have any sources? Where do you have it from?

@deitch and I remarked that the function name is a bit verbose and might be changed to something like AllPathsBetween before the release, but for now, this looks good to me. 👍

Comment on lines +256 to +269
for e := range adjacencyMap[element] {
var contains bool
mainStack.forEach(func(k K) {
if e == k {
contains = true
}
})
if contains {
continue
}
newElements.push(e)
}
viceStack.push(newElements)
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be simplified using a loop label:

adjacencies:
		for e := range adjacencyMap[element] {
			mainStack.forEach(func(k K) {
				if e == k {
					continue adjacencies
				}
			})
			newElements.push(e)
		}
		viceStack.push(newElements)
	}

It isn't that important though, because I'm about to add a stack.contains method later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the idea from this blog (but I have not found the English-described version and the pictures in the blog has some mistakes).

I have renamed the function to AllPathsBetween.

I considered adding the method stack.contains. However, I'm not sure whether it's good or not to define the generic type of stack as comparable rather than any. Thus, I trickly used the method forEach. If a contains method is added to stack, this block of code will be nicer ~

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tzq0301 Thanks for the link to the blog post!

I considered adding the method stack.contains. However, I'm not sure whether it's good or not to define the generic type of stack as comparable rather than any.

I see. Since stack is only being used internally and only deals with hash values of type K comparable anyway, I think it is fine to constrain it to comparable.

@dominikbraun dominikbraun self-requested a review July 4, 2023 13:38
Copy link
Copy Markdown
Owner

@dominikbraun dominikbraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks alot!

@dominikbraun dominikbraun merged commit 92fa587 into dominikbraun:main Jul 4, 2023
@dominikbraun
Copy link
Copy Markdown
Owner

This change has been released in graph v0.23.0.

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.

2 participants