[WIP] Add FloydWarshall Algorithm (All pairs shortest path) #71
Conversation
|
Thanks a bunch for providing a new algorithm! Looks really good and I'd love to see it included 👍 Given the fact it's an all pairs shortest paths algorithm, it differs significantly from the other current single-source shortest paths implementations (Dijkstra, Moore-Bellman-Ford, Breadth-First). As such, I think it warrants a more thorough feedback and some adjustments to our current code structure. Afaikt what needs to be done to our current code base before the algorithm fits our current design:
Other than that, I think your implementation looks really promising! I'll add a few remarks to the code layout for some things that come to mind. Most of the remarks probably stem for the fact that this is the first all pairs shortest paths algorithm, so please don't take those personally as it's a natural thing we have to work out some of the differences. Most of this will probably also be needed in order to provide some documentation on which use-case each algorithm covers and how it handles some edge cases (such as null graphs, edgeless graphs, negative weights, parallel edges, multiple components, etc.) Any thoughts besides this? @clemens-tolboom, care to take look? |
| * An implementation of the Floyd Warshall algorithm to find the shortest path from each vertex to each vertex. | ||
| * @link http://en.wikipedia.org/wiki/Floyd%E2%80%93Warshall_algorithm | ||
| * | ||
| * @package Fhaculty\Graph\Algorithm\ShortestPath |
There was a problem hiding this comment.
- Description should be wrapped at 80 chars (see e.g. Dijkstra's description)
- First line should be more descriptive
- No need to define package :) (redundant with namespace)
- Extend description to describe some edge cases (related to what was done in Improve Algorithm\ShortestPath #62)
- Distance between isolated Vertices?
- Are parallel (multiple) edges supported?
- Expected results for negative edge weights?
- Expected results for negative cycles?
- Expected results for null Graph (no vertices), edgeless Graph?
- Distance between Vertex A and Vertex A? What about Loops? What about negative Loops?
There was a problem hiding this comment.
Thanks for the answer!, I'm working on some changes to the algorithm according to your comments, as soon as possible you will see them.
There was a problem hiding this comment.
Glad to hear! 👍 Just keep me updated once you want me to review your results.
…hrow an Exception in case of negative cycles (or loops), the Tests were also rewritten to be more specific.
|
This commit implemented a Result interface for handling the Result of the FloydWarshall Algorithm better. |
|
Thanks for the update, these changes look really good! 👍 In particular the amount and quality of documentation and unit tests is very much appreciated! I'm quite busy for the next few days, so I'm a afraid a more thorough review may take some time, but I'll come back to this ticket ASAP. What's your status on this ticket? What do you think still has to be done before this should be merged? |
| $vertex[0] = self::$graphs[1]->createVertex('A'); | ||
| $vertex[1] = self::$graphs[1]->createVertex('B'); | ||
| $vertex[0]->createEdgeTo($vertex[1]); | ||
| $edges = $vertex[0]->getEdgesTo($vertex[1]); |
There was a problem hiding this comment.
Personally, I'd don't quite like the style, but the failing test can be fixed with:
$edges = $vertex[0]->getEdgesTo($vertex[1])->getVector()
|
So, I've finally had the time to take a more thorough look at your PR. And I really like the high quality of it and would love to merge it once the remaining remarks have been resolved. Sorry for bugging you with those (sometimes subtle) remarks :) I'd love to hear back from you! |
|
Hello Thank you for your attention. I am a bit busy right now, but I will make the changes as soon as possible. I hope to have a new pull request soon. |
|
No worries, glad to hear we'll eventually manage to sort out the remaining remarks :)
Sure, keep them coming 👍 |
…ithm classes moved to new namespace, and Result class had the createGraph and getVertices methods rewritten.
|
Hello. I made a new pull request, I hope the new changes will resolve the remarks you stated. Thank you. |
|
The changes look good to me. Unfortunately, there's still a failing unit test. Is there anything you'd like to address yourself or would you want me to dig a little deeper? :) Thanks! |
|
Hello. Sorry for the late answer, I've been very busy these months. I will re-check the code, if you could give information about the test that If everything goes ok, you should be receiving a new pull request soon. Thank you. On Sun, Dec 29, 2013 at 9:13 PM, Christian Lück notifications@git.luolix.topwrote:
"porque el fracaso es como el exito...pero al reves" |
No worries, we all face that same issue :)
You can either run phpunit locally or take a look at travis output: Otherwise, we also usually hang around on #graphp on irc.freenode.net, so feel free to stop by. The master branch has advanced quite a bit, most notably this code has to be changed: // old:
$array = $vertex->getEdges();
// new:
$array = $vertex->getEdges()->getVector();
As always, thanks for your work, it's very much appreciated! |
|
Thanks for your interest! All algorithms will be split off to a separate package graphp/algorithms (see also #119 for some background). As such, this PR can no longer be merged into this repo. That being said, I'd still love this see this in! 👍 I've filed a new ticket as a reminder to keep track of this PR until it has been migrated over. |
First attemps to create a FloydWarshall class for creating an Edge list for each index of an associative matrix representing the shortest path between two connected vertices.