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

[WIP] Support undirected graphs for Algorithm\ShortestPath #79

Closed
wants to merge 14 commits into from

Conversation

clue
Copy link
Member

@clue clue commented Nov 14, 2013

This PR is a WIP ticket to keep track of the current status to actually support undirected graphs for Dijkstra and MooreBellmanFord.

  • Implement fix
  • 100% coverage
  • Documentation / Changelog

Refs #66 (original bug report + lengthy details on what needs to be done) and supersedes #68 (initial attempt).

Each algorithm only implements the new method which returns an
intermediary result. The dedicated Result interface implements the
common accessors and avoids running the algorithm again.
Added a BC layer for now, which forwards all method calls.
The new algorithms now use a smaller signature and avoid passing the
start Vertex to the ctor.
Keep a BC layer for now.
Conflicts:
	lib/Fhaculty/Graph/Algorithm/ShortestPath/MooreBellmanFord.php
	tests/Fhaculty/Graph/Algorithm/ShortestPath/BaseShortestPathTest.php
	tests/Fhaculty/Graph/Algorithm/ShortestPath/MooreBellmanFordTest.php
The new design allows for a much cleaner implementation. This now also
works for undirected edges just as well.

Fixes #66
@clue
Copy link
Member Author

clue commented Dec 31, 2013

The good news:

  • The issue in PR Shortest path depends on vertex sequence #66 has finally been fixed, i.e. all shortest path algorithm now support undirected edges just as well.
  • As a prerequisite, all algorithms have been updated to return an intermediary Result object.
  • As such, the API to each algorithm has been updated, simplified and improved.
  • This PR also adjusts the directory structure in accordance to PR [WIP] Add FloydWarshall Algorithm (All pairs shortest path) #71 so that all shortest path algorithms have been moved to the new namespace Algorithm\ShortestPath\SingleSource.
  • Everything works flawlessly and has 100% test coverage

The bad news:

  • Obviously, given the large number of changes this is quite a mess
  • The individual changesets are too big, and git didn't pick up the renames
  • As such, I've introduced a couple of merge conflicts which I failed to merge properly (some changes from master were lost)
  • A huge BC break (which I'm okay with), so this will need a fair bit of documentation

So while the end result looks good to me, it's quite difficult to work with. To clean things up a bit, I'll have to rework this whole thing and introduce smaller, independent changesets. Obviously, this will take some time.

So if anybody wants to test out the results and check if this branch fixes #66, go ahead :) But expect this to take some time until it can actually be merged.

@clue
Copy link
Member Author

clue commented Feb 25, 2015

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.

@clue clue closed this Feb 25, 2015
@clue clue deleted the shortestpath-undirected branch February 25, 2015 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant