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

Replace recursive topological sort with iterative algorithm #25

Merged
merged 1 commit into from
Oct 10, 2018
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 37 additions & 28 deletions src/TopologicalSort.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,47 +19,56 @@ class TopologicalSort extends BaseGraph
/**
* run algorithm and return an ordered/sorted set of Vertices
*
* the topologic sorting may be non-unique depending on your edges. this
* algorithm tries to keep the order of vertices as added to the graph in
* this case.
* the topological sorting may be non-unique depending on your edges
Copy link
Member

Choose a reason for hiding this comment

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

Was this removed intentionally? If so, should this be considered a BC break? Can you add some test cases for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it should be considered BC - I'm not sure the topsort should not have made any potential promises about the ordering in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure the topsort should not have made any potential promises about the ordering in the first place.

I agree that it should not have made this promise in the first place.

That being said, I suppose this is something some projects may currently rely on it. As such, I'm marking this as a BC break just in case and will merge this as-is.

Again, thank you! 👍

*
* @return Vertices
*/
public function getVertices()
{
$tsl = array();
$stack = array(); // visited nodes with unvisited children
$visited = array();
$output = array();

// TODO: find alternative to recursive algorithm to avoid hitting recursion limit with ~100 nodes
/** @var Vertex $top */
// TODO: avoid having to reverse all vertices multiple times
// pick a node to examine next - assume there are isolated nodes
foreach (array_reverse($this->graph->getVertices()->getVector()) as $top) {
$tid = $top->getId();
if (!isset($visited[$tid])) { // don't examine if already found
array_push($stack, $top);
}

foreach(array_reverse($this->graph->getVertices()->getVector()) as $vertex) {
$this->visit($vertex, $visited, $tsl);
}
while ($stack) {
/** @var Vertex $current */
$node = end($stack);
$nid = $node->getId();

return new Vertices(array_reverse($tsl, true));
}
$visited[$nid] = false; // temporary mark
$found = false; // new children found during visit to this node

protected function visit(Vertex $vertex, array &$visited, array &$tsl)
{
$vid = $vertex->getId();
if (isset($visited[$vid])) {
if ($visited[$vid] === false) {
// temporary mark => not a DAG
throw new UnexpectedValueException('Not a DAG');
}
// otherwise already marked/visisted => no need to check again
} else {
// temporary mark
$visited[$vid] = false;
// find the next node to visit
/** @var Vertex $child */
foreach (array_reverse($node->getVerticesEdgeTo()->getVector()) as $child) {
$cid = $child->getId();
if (!isset($visited[$cid])) {
// found a new node - push it onto the stack
array_push($stack, $child);
$found = true; // move onto the new node
break;
} else if ($visited[$cid] === false) {
// temporary mark => not a DAG
throw new UnexpectedValueException('Not a DAG');
}
}

foreach (array_reverse($vertex->getVerticesEdgeTo()->getVector()) as $v) {
$this->visit($v, $visited, $tsl);
if (!$found) {
array_pop($stack); // no new children found - we're done with this node
$visited[$nid] = true; // mark as visited
array_push($output, $node); // add to results
}
}

// mark as visited and include in result
$visited[$vid] = true;
$tsl[$vid] = $vertex;
}

return new Vertices(array_reverse($output, true));
}
}