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

Tarjan's strongly connected component and Shortest Cycle finder algorithm #106

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 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
57 changes: 57 additions & 0 deletions lib/Fhaculty/Graph/Algorithm/CycleFinder.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php

namespace Fhaculty\Graph\Algorithm;

use Fhaculty\Graph\Algorithm\ShortestPath\Dijkstra;
use Fhaculty\Graph\Algorithm\StronglyConnectedComponents\Tarjan;
use Fhaculty\Graph\Walk;
use Fhaculty\Graph\Graph;

class CycleFinder extends BaseGraph
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this class and its tests is independent of the other changes? If so, would you care to make this a separate PR?

New algorithms are very much appreciated, keeping them separated helps reviewing the changes and will get things in a lot easier 👍

{
/**
* @see (not used) http://stones333.blogspot.fr/2013/12/find-cycles-in-directed-graph-dag.html
* @see http://stackoverflow.com/questions/10456935/graph-how-to-find-minimum-directed-cycle-minimum-total-weight
*/
public function getShortestCycle()
{
$walkMap = array();
$vertices = $this->graph->getVertices()->getList();

// Compute distance map
foreach($vertices as $u){
foreach($vertices as $v){
if($u === $v){
Copy link
Member

Choose a reason for hiding this comment

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

Minor CS issue. Should be the following as per PSR-2:

foreach ($vertices as $v) {
    if ($u === $v) {

continue;
}
if(isset($walkMap[$u->getId()][$v->getId()])){
continue;
}
$distUV = new Dijkstra($u);
$walkMap[$u->getId()][$v->getId()] = $distUV->getWalkTo($v);
}
}

// The eulerian path is the shortest
$minimum = count($this->graph->getEdges()) + 1;
$walk = null;

// Find shortest
foreach($vertices as $u){
foreach($vertices as $v){
if($u === $v){
continue;
}
$walkU = $walkMap[$u->getId()][$v->getId()];
$walkV = $walkMap[$v->getId()][$u->getId()];
$length = $walkU->getLength() + $walkV->getLength();
if($length < $minimum){
$minimum = $length;
$walk = $walkU->append($walkV);
}
}
}

return $walk;
Copy link
Member

Choose a reason for hiding this comment

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

This method has a null return value if no Walk could be found. IMHO this should be an UnderflowException (consistency).

}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs a new line I guess.

100 changes: 100 additions & 0 deletions lib/Fhaculty/Graph/Algorithm/StronglyConnectedComponents/Tarjan.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
<?php

namespace Fhaculty\Graph\Algorithm\StronglyConnectedComponents;

use Fhaculty\Graph\Algorithm\BaseGraph;
use Fhaculty\Graph\Algorithm\Directed;
use Fhaculty\Graph\Exception\InvalidArgumentException;
use Fhaculty\Graph\Graph;
use Fhaculty\Graph\Set\Vertices;
use Fhaculty\Graph\Vertex;
use Fhaculty\Graph\Set\VerticeDataMap;

/**
* @see http://github.com/Trismegiste/Mondrian/blob/master/Graph/Tarjan.php
*/
class Tarjan extends BaseGraph
{
private $stack;
private $index;
private $partition;

private $indexMap = array();
private $lowLinkMap = array();

/**
* @param Graph $graph
*/
public function __construct(Graph $graph)
{
parent::__construct($graph);
$this->indexMap = new VerticeDataMap();
$this->lowLinkMap = new VerticeDataMap();
}

/**
* Get the strongly connected components of this digraph by
* the Tarjan algorithm.
*
* Starting from :
* http://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm
*
* Corrected with the help :
* https://code.google.com/p/jbpt/source/browse/trunk/jbpt-core/src/main/java/org/jbpt/algo/graph/StronglyConnectedComponents.java
* @throws \Fhaculty\Graph\Exception\InvalidArgumentException
* @return array the partition of this graph : an array of an array of vertices
*/
public function getStronglyConnected()
{

// check is directed
$directed = new Directed($this->graph);
if($directed->hasUndirected()){
throw new InvalidArgumentException('Graph shall be directed');
}

$this->stack = array();
$this->index = 0;
$this->partition = array();

foreach ($this->graph->getVertices()->getList() as $vertex) {
if (! isset($this->indexMap[$vertex])) {
$this->recursivStrongConnect($vertex);
}
}

return $this->partition;
}

private function recursivStrongConnect(Vertex $v)
{
$this->indexMap[$v] = $this->index;
$this->lowLinkMap[$v] = $this->index;
$this->index++;
array_push($this->stack, $v);

// Consider successors of v
foreach ($v->getVerticesEdgeTo() as $w) {
if (! isset($this->indexMap[$w]) ) {
// Successor w has not yet been visited; recurse on it
$this->recursivStrongConnect($w);
$this->lowLinkMap[$v] = min(array($this->lowLinkMap[$v], $this->lowLinkMap[$w]));
} elseif (in_array($w, $this->stack)) {
// Successor w is in stack S and hence in the current SCC
$this->lowLinkMap[$v] = min(array($this->lowLinkMap[$v], $this->indexMap[$w]));
}
}
// If v is a root node, pop the stack and generate an SCC
if ($this->lowLinkMap[$v] === $this->indexMap[$v]) {
$scc = array();
do {
$w = array_pop($this->stack);
array_push($scc, $w);
} while ($w !== $v);

if (count($scc)) {
$this->partition[] = new Vertices($scc);
}
}
}
}
4 changes: 2 additions & 2 deletions lib/Fhaculty/Graph/Edge/Base.php
Original file line number Diff line number Diff line change
Expand Up @@ -250,9 +250,9 @@ public function getGraph()
*/
public function destroy()
{
$this->getGraph()->removeEdge($this);
$this->getGraph()->removeEdge($this, false);
foreach ($this->getVertices() as $vertex) {
$vertex->removeEdge($this);
$vertex->removeEdge($this, false);
}
}

Expand Down
14 changes: 9 additions & 5 deletions lib/Fhaculty/Graph/Graph.php
Original file line number Diff line number Diff line change
Expand Up @@ -362,14 +362,18 @@ public function addEdge(Edge $edge)
* @private
* @see Edge::destroy() instead!
*/
public function removeEdge(Edge $edge)
public function removeEdge(Edge $edge, $throwException = true)
Copy link
Member

Choose a reason for hiding this comment

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

Would you care to elaborate what this change is needed for?

Semantically this makes it a "remove edge if it exists" method and I'd rather keep this code explicitly say "remove edge or fail". Besides, adding a boolean parameter looks like a boolean trap to me.

Copy link

Choose a reason for hiding this comment

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

Well, I don't recall right now. I'll keep this separated in another PR, it does not seem to be mandatory for tarjan.

{
try {
unset($this->edgesStorage[$this->edges->getIndexEdge($edge)]);
$id = $this->edges->getIndexEdge($edge);
if ($id === false) {
if($throwException){
throw new InvalidArgumentException('Invalid Edge does not exist in this Graph');
}
}
catch (OutOfBoundsException $e) {
throw new InvalidArgumentException('Invalid Edge does not exist in this Graph');
else{
unset($this->edgesStorage[$id]);
}

}

/**
Expand Down
7 changes: 1 addition & 6 deletions lib/Fhaculty/Graph/Set/Edges.php
Original file line number Diff line number Diff line change
Expand Up @@ -117,16 +117,11 @@ public function __construct(array $edges = array())
* get array index for given Edge
*
* @param Edge $edge
* @throws OutOfBoundsException
* @return mixed
*/
public function getIndexEdge(Edge $edge)
{
$id = array_search($edge, $this->edges, true);
if ($id === false) {
throw new OutOfBoundsException('Given edge does NOT exist');
}
return $id;
return array_search($edge, $this->edges, true);
Copy link
Member

Choose a reason for hiding this comment

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

This change makes it work inconsistent with regards to Vertices::getIndexVertex(). What's the rationale for this change?

}

/**
Expand Down
43 changes: 43 additions & 0 deletions lib/Fhaculty/Graph/Set/VerticeDataMap.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php


namespace Fhaculty\Graph\Set;

class VerticeDataMap implements \ArrayAccess
Copy link
Member

Choose a reason for hiding this comment

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

Admittedly I'm failing to see the need for the VerticeDataMap class, perhaps you would care to elaborate / document this a bit?

Copy link
Author

Choose a reason for hiding this comment

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

I found the need to attach metadata to Vertex. For example, if the graph represents a dependency tree, each vertex carries informations about package, url, and so on. It's great to embed the information in the system rather than duplicate the graph somewhere else. It sure does need more documentation.

Copy link

Choose a reason for hiding this comment

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

Separate PR ;)

{
private $dataMap = array();

/**
* {@inheritdoc}
*/
public function offsetExists($offset)
{
return isset($this->dataMap[$offset->getId()]);
}

/**
* {@inheritdoc}
*/
public function offsetGet($offset)
{
return $this->dataMap[$offset->getId()];
}

/**
* {@inheritdoc}
*/
public function offsetSet($offset, $value)
{
$this->dataMap[$offset->getId()] = $value;
return $this;
}

/**
* {@inheritdoc}
*/
public function offsetUnset($offset)
{
unset($this->dataMap[$offset->getId()]);
}

}
5 changes: 5 additions & 0 deletions lib/Fhaculty/Graph/Set/VerticesMap.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@
*/
class VerticesMap extends Vertices
{
public function getList()
Copy link
Member

Choose a reason for hiding this comment

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

Could this be a duplicate of the getVector() method? If so, sorry for the bad documentation :)

Copy link
Author

Choose a reason for hiding this comment

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

It is totally a duplicate, my bad :)

{
return array_values($this->getMap());
}

public function getMap()
{
return $this->vertices;
Expand Down
10 changes: 7 additions & 3 deletions lib/Fhaculty/Graph/Vertex.php
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,17 @@ public function addEdge(Edge $edge)
* @private
* @see Edge::destroy() instead!
*/
public function removeEdge(Edge $edge)
public function removeEdge(Edge $edge, $throwException = true)
Copy link
Member

Choose a reason for hiding this comment

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

See also my other comment.

{
$id = array_search($edge, $this->edges, true);
if ($id === false) {
throw new InvalidArgumentException('Given edge does NOT exist');
if($throwException){
throw new InvalidArgumentException('Given edge does NOT exist');
}
}
else{
unset($this->edges[$id]);
}
unset($this->edges[$id]);
}

/**
Expand Down
38 changes: 38 additions & 0 deletions lib/Fhaculty/Graph/Walk.php
Original file line number Diff line number Diff line change
Expand Up @@ -310,4 +310,42 @@ public function isValid()

return true;
}

public function getLength()
Copy link
Member

Choose a reason for hiding this comment

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

Arguably this is a useful addition. Graph theory commonly refers to the number of edges as the length or size of a walk.

However, I suppose this might be misleading with regards to a Walks vs. Paths and also might be mistaken as counting the number of vertices.

Either way, we should consider if we can avoid this ambiguity by not providing this method at all and asking users to explicitly call $length = count($walk->getEdges()) instead.

{
return count($this->getEdges());
}

public function __toString()
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change? I'd much rather try to keep the number of changes per PR low.

If you feel this is needed, make sure to read through #23 and consider filing this as a separate PR?

Copy link

Choose a reason for hiding this comment

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

Okay removing them, I'll open a new PR later.

{
$ret = array();
foreach($this->getEdges() as $e){
array_push($ret, sprintf('%s -> %s',
$e->getVertexStart()->getId(),
$e->getVertexEnd()->getId()
));
}
return implode(', ', $ret);
}

/**
* @param Walk $walk
*/
public function append(Walk $walk)
Copy link
Member

Choose a reason for hiding this comment

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

Nice addition! 👍

In my opinion this needs some more attention and probably makes sense as a separate PR so we can track this as a separate feature.

Copy link

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

For the reference: PR filed as #112.

{
// first edge need to be starting at the same point.
if(
$this->getVertices()->getVertexFirst() !==
$walk->getVertices()->getVertexLast()
) {
throw new InvalidArgumentException('First vertex of $walk shall be the same as the last one');
}

$vertices = array_merge(
$this->getVertices()->getVector(),
array_slice($walk->getVertices()->getVector(), 1)
);

return self::factoryCycleFromVertices($vertices);
}
}
35 changes: 35 additions & 0 deletions tests/Fhaculty/Graph/Algorithm/CycleFinderTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

use Fhaculty\Graph\Graph;

use Fhaculty\Graph\Algorithm\CycleFinder;
use Fhaculty\Graph\Algorithm\StronglyConnectedComponents\Tarjan;
use Fhaculty\Graph\Walk;

class CycleFinderTest extends TestCase
{
public function testGraphHaSCycle()
{
$graph = new Graph();

$A = $graph->createVertex("A");
$B = $graph->createVertex("B");
$C = $graph->createVertex("C");

$A->createEdgeTo($B);
$B->createEdgeTo($C);
$C->createEdgeTo($A);
$B->createEdgeTo($A);

// $tarjan = new Tarjan($graph);
// $componentsVertices = $tarjan->getStronglyConnected();
// foreach($componentsVertices as $componentVertices){
// $component = $graph->createGraphCloneVertices($componentVertices);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these lines commented?


$find = new CycleFinder($graph);
$shortest = $find->getShortestCycle();

$walk = Walk::factoryCycleFromVertices(array($A, $B, $A));
$this->assertSame($walk->__toString(), $shortest->__toString());
}
}
Loading