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

Dfs side effect #43

Closed

Conversation

ferranpujolcamins
Copy link
Collaborator

This PR generalizes the search algorithm so both DFS and BFS use the same core algorithm.
It also introduces new methods to perform arbitrary computations over a graph.

A new struct Search is added, which is used to configure the behaviour of the core algorithm to obtain either bfs or dfs. This Search struct can be the base for a more elaborate domain specific language to configure a search algorithm, but right know I have kept things simple.

Convenience DFS and BFS type alias over Search are provided to quickly configure these specific algorithms.

The previous graph search api coded in Graph extension is mantained, but internally uses the new Search API.

This is a rather big change, I apologize for just throwing the PR withut previous discussion. But the commits are self-contained and the abstraction is introduced progressively, so I hope you can just inspect commit by commit to see what's going on. If you don't feel confident with all the changes, let me know, I will cherry-pick the ones that are ok. Let's talk :)

@ferranpujolcamins ferranpujolcamins force-pushed the DFS-SideEffect branch 2 times, most recently from e2c2a21 to 8bf9f52 Compare October 10, 2018 17:01
@davecom
Copy link
Owner

davecom commented Oct 11, 2018

What do you think are the pros and cons of this approach over the existing one? This adds some complexity and I imagine some performance cost. On the other hand, it creates a congruous interface that can be updated for both BFS and DFS. Is that pretty much right? Do you think we should do some performance tests looking at this versus the old implementation?

@davecom
Copy link
Owner

davecom commented Oct 11, 2018

How easily do you think we can add A* to this? I have an example of A* in my other project, SwiftPriorityQueue:
https://github.com/davecom/SwiftPriorityQueue/blob/master/SwiftPriorityQueue/astar.swift

@davecom
Copy link
Owner

davecom commented Oct 11, 2018

And also thanks for the contribution in advance.

@ferranpujolcamins
Copy link
Collaborator Author

You are totally right, on a star graph, a full dfs traversal is 38% slower with the new implementation.
I'll try to profile and see how can I reduce times, maybe with generics and static dispatch.
Let's put this PR on hold until I manage to improve the speed. I'll keep commiting to this branch, and I'll rebase to clean the commit history when done.

@ferranpujolcamins
Copy link
Collaborator Author

The A* seems doable within the new implementation at first glance.

@davecom
Copy link
Owner

davecom commented Oct 13, 2018

Thanks for doing the research on the performance cost. And I also appreciate all the work you put into this approach—it's cool. Yeah, I doubt anyone is using SwiftGraph for anything that requires high performance, but I don't see a reason to introduce a degradation. If it turns out we can't improve the performance perhaps we can just add A* to the base project.

One lesson I learned from this: we should probably add performance tests into the unit tests for the project.

@ferranpujolcamins
Copy link
Collaborator Author

Actually, when optimization is turned on, the new implementation is even worse: 44% slower 😞

# Conflicts:
#	SwiftGraph.xcodeproj/project.pbxproj
@ferranpujolcamins
Copy link
Collaborator Author

I'm closing this PR by now (I'll reopen it If I can fix the drop in performance).

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.

None yet

2 participants