diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a3303af..5a0298d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,19 @@ you spot any mistakes. ## 0.7.0 (2013-xx-xx) +* BC break: Merge `Cycle` into `Walk` ([#61](https://github.com/clue/graph/issues/61)). +As such, its static factory methods had to be renamed. Update your references if applicable: + +| Old name | New name | +|---|---| +| `Cycle::factoryFromPredecessorMap()` | `Walk::factoryCycleFromPredecessorMap()` | +| `Cycle::factoryFromVertices()` | `Walk::factoryCycleFromVertices()` | +| `Cycle::factoryFromEdges()` | `Walk::factoryCycleFromEdges()` | + +* BC break: Each of the above methods (`Walk::factoryCycleFromPredecessorMap()`, +`Walk::factoryCycleFromVertices()`, `Walk::factoryCycleFromEdges()`) now +actually makes sure the returned `Walk` instance is actually a valid Cycle, +i.e. the start `Vertex` is the same as the end `Vertex` ([#61](https://github.com/clue/graph/issues/61)) * BC break: Each `Algorithm\ShortestPath` algorithm now consistenly does not return a zero weight for the root Vertex and now supports loop edges on the root Vertex ([#62](https://github.com/clue/graph/issues/62)) @@ -14,6 +27,7 @@ Vertex ([#62](https://github.com/clue/graph/issues/62)) ([#62](https://github.com/clue/graph/issues/62)) * Feature: Add `Algorithm\ShortestPath::hasVertex(Vertex $vertex)` to check whether a path to the given Vertex exists ([#62](https://github.com/clue/graph/issues/62)). +* Fix: Checking `Walk::isValid()` ([#61](https://github.com/clue/graph/issues/61)) * Fix: Missing import prevented `Algorithm\ShortestPath\MooreBellmanFord::getCycleNegative()` from actually throwing the right `UnderflowException` if no cycle was actually found diff --git a/lib/Fhaculty/Graph/Algorithm/DetectNegativeCycle.php b/lib/Fhaculty/Graph/Algorithm/DetectNegativeCycle.php index c3260859..d2bac5ee 100644 --- a/lib/Fhaculty/Graph/Algorithm/DetectNegativeCycle.php +++ b/lib/Fhaculty/Graph/Algorithm/DetectNegativeCycle.php @@ -7,6 +7,7 @@ use Fhaculty\Graph\Graph; use Fhaculty\Graph\Vertex; +use Fhaculty\Graph\Walk; use Fhaculty\Graph\Exception\NegativeCycleException; use Fhaculty\Graph\Algorithm\ShortestPath\MooreBellmanFord as SpMooreBellmanFord; @@ -34,7 +35,7 @@ public function hasCycleNegative() /** * Searches all vertices for the first negative cycle * - * @return Cycle + * @return Walk * @throws UnderflowException if there's no negative cycle * @uses AlgorithmSpMooreBellmanFord::getVerticesId() */ @@ -72,7 +73,7 @@ public function getCycleNegative() * @return Graph * @throws Exception if there's no negative cycle * @uses AlgorithmDetectNegativeCycle::getCycleNegative() - * @uses Cycle::createGraph() + * @uses Walk::createGraph() */ public function createGraph() { diff --git a/lib/Fhaculty/Graph/Algorithm/ShortestPath/MooreBellmanFord.php b/lib/Fhaculty/Graph/Algorithm/ShortestPath/MooreBellmanFord.php index 97124521..70fabe7f 100644 --- a/lib/Fhaculty/Graph/Algorithm/ShortestPath/MooreBellmanFord.php +++ b/lib/Fhaculty/Graph/Algorithm/ShortestPath/MooreBellmanFord.php @@ -3,7 +3,7 @@ namespace Fhaculty\Graph\Algorithm\ShortestPath; use Fhaculty\Graph\Edge\Base as Edge; -use Fhaculty\Graph\Cycle; +use Fhaculty\Graph\Walk; use Fhaculty\Graph\Exception\NegativeCycleException; use Fhaculty\Graph\Exception\UnderflowException; @@ -97,7 +97,7 @@ public function getEdges() // Check for negative cycles (only if last step didn't already finish anyway) // something is still changing... if ($changed && $changed = $this->bigStep($edges, $totalCostOfCheapestPathTo, $predecessorVertexOfCheapestPathTo)) { - $cycle = Cycle::factoryFromPredecessorMap($predecessorVertexOfCheapestPathTo, $changed, Edge::ORDER_WEIGHT); + $cycle = Walk::factoryCycleFromPredecessorMap($predecessorVertexOfCheapestPathTo, $changed, Edge::ORDER_WEIGHT); throw new NegativeCycleException('Negative cycle found', 0, NULL, $cycle); } @@ -107,7 +107,7 @@ public function getEdges() /** * get negative cycle * - * @return Cycle + * @return Walk * @throws UnderflowException if there's no negative cycle */ public function getCycleNegative() diff --git a/lib/Fhaculty/Graph/Algorithm/TravelingSalesmanProblem/Base.php b/lib/Fhaculty/Graph/Algorithm/TravelingSalesmanProblem/Base.php index 5720bb97..22a24fff 100644 --- a/lib/Fhaculty/Graph/Algorithm/TravelingSalesmanProblem/Base.php +++ b/lib/Fhaculty/Graph/Algorithm/TravelingSalesmanProblem/Base.php @@ -2,7 +2,7 @@ namespace Fhaculty\Graph\Algorithm\TravelingSalesmanProblem; -use Fhaculty\Graph\Cycle; +use Fhaculty\Graph\Walk; use Fhaculty\Graph\Vertex; use Fhaculty\Graph\Edge\Base as Edge; use Fhaculty\Graph\Algorithm\Base as AlgorithmBase; @@ -40,14 +40,14 @@ abstract protected function getVertexStart(); /** * get (first) best circle connecting all vertices * - * @return Cycle + * @return Walk * @uses AlgorithmTsp::getEdges() * @uses AlgorithmTsp::getVertexStart() - * @uses Cycle::factoryFromEdges() + * @uses Walk::factoryCycleFromEdges() */ public function getCycle() { - return Cycle::factoryFromEdges($this->getEdges(), $this->getVertexStart()); + return Walk::factoryCycleFromEdges($this->getEdges(), $this->getVertexStart()); } public function getWeight() diff --git a/lib/Fhaculty/Graph/Cycle.php b/lib/Fhaculty/Graph/Cycle.php deleted file mode 100644 index ead2eb52..00000000 --- a/lib/Fhaculty/Graph/Cycle.php +++ /dev/null @@ -1,114 +0,0 @@ - predecessor vertex instance - * @param Vertex $vertex start vertex to search predecessors from - * @param int $by - * @param boolean $desc - * @return Cycle - * @throws UnderflowException - * @see Edge::getFirst() for parameters $by and $desc - * @uses Cycle::factoryFromVertices() - */ - public static function factoryFromPredecessorMap($predecessors, $vertex, $by = Edge::ORDER_FIFO, $desc = false) - { - /*$checked = array(); - foreach ($predecessors as $vertex) { - $vid = $vertex->getId(); - if (!isset($checked[$vid])) { - - } - }*/ - - // find a vertex in the cycle - $vid = $vertex->getId(); - $startVertices = array(); - do { - $startVertices[$vid] = $vertex; - - $vertex = $predecessors[$vid]; - $vid = $vertex->getId(); - } while (!isset($startVertices[$vid])); - - // find negative cycle - $vid = $vertex->getId(); - // build array of vertices in cycle - $vertices = array(); - do { - // add new vertex to cycle - $vertices[$vid] = $vertex; - - // get predecessor of vertex - $vertex = $predecessors[$vid]; - $vid = $vertex->getId(); - // continue until we find a vertex that's already in the circle (i.e. circle is closed) - } while (!isset($vertices[$vid])); - - // reverse cycle, because cycle is actually built in opposite direction due to checking predecessors - $vertices = array_reverse($vertices, true); - - return Cycle::factoryFromVertices($vertices, $by, $desc); - } - - /** - * create new cycle instance with edges between given vertices - * - * @param Vertex[] $vertices - * @param int $by - * @param boolean $desc - * @return Cycle - * @throws UnderflowException if no vertices were given - * @see Edge::getFirst() for parameters $by and $desc - */ - public static function factoryFromVertices($vertices, $by = Edge::ORDER_FIFO, $desc = false) - { - $edges = array(); - $first = NULL; - $last = NULL; - foreach ($vertices as $vertex) { - // skip first vertex as last is unknown - if ($first === NULL) { - $first = $vertex; - } else { - // pick edge between last vertex and this vertex - $edges []= Edge::getFirst($last->getEdgesTo($vertex), $by, $desc); - } - $last = $vertex; - } - if ($last === NULL) { - throw new UnderflowException('No vertices given'); - } - // additional edge from last vertex to first vertex - $edges []= Edge::getFirst($last->getEdgesTo($first), $by, $desc); - - return new Cycle($vertices, $edges); - } - - /** - * create new cycle instance with vertices connected by given edges - * - * @param Edge[] $edges - * @param Vertex $startVertex - * @return Cycle - */ - public static function factoryFromEdges(array $edges, Vertex $startVertex) - { - $vertices = array($startVertex->getId() => $startVertex); - foreach ($edges as $edge) { - $vertex = $edge->getVertexToFrom($startVertex); - $vertices[$vertex->getId()] = $vertex; - $startVertex = $vertex; - } - - return new Cycle($vertices, $edges); - } -} diff --git a/lib/Fhaculty/Graph/Exception/NegativeCycleException.php b/lib/Fhaculty/Graph/Exception/NegativeCycleException.php index 3f9d174a..de8eef07 100644 --- a/lib/Fhaculty/Graph/Exception/NegativeCycleException.php +++ b/lib/Fhaculty/Graph/Exception/NegativeCycleException.php @@ -2,8 +2,7 @@ namespace Fhaculty\Graph\Exception; -use Fhaculty\Graph\Cycle; - +use Fhaculty\Graph\Walk; use Fhaculty\Graph; class NegativeCycleException extends UnexpectedValueException implements Graph\Exception @@ -11,11 +10,11 @@ class NegativeCycleException extends UnexpectedValueException implements Graph\E /** * instance of the cycle * - * @var Cycle + * @var Walk */ private $cycle; - public function __construct($message, $code = NULL, $previous = NULL, Cycle $cycle) + public function __construct($message, $code = NULL, $previous = NULL, Walk $cycle) { parent::__construct($message, $code, $previous); $this->cycle = $cycle; @@ -23,7 +22,7 @@ public function __construct($message, $code = NULL, $previous = NULL, Cycle $cyc /** * - * @return Cycle + * @return Walk */ public function getCycle() { diff --git a/lib/Fhaculty/Graph/Walk.php b/lib/Fhaculty/Graph/Walk.php index b50049c0..13737c7b 100644 --- a/lib/Fhaculty/Graph/Walk.php +++ b/lib/Fhaculty/Graph/Walk.php @@ -2,6 +2,10 @@ namespace Fhaculty\Graph; +use Fhaculty\Graph\Edge\Base as Edge; +use Fhaculty\Graph\Exception\UnderflowException; +use Fhaculty\Graph\Exception\InvalidArgumentException; + /** * Base Walk class * @@ -19,7 +23,7 @@ class Walk extends Set * * @param array $edges * @param Vertex $startVertex - * @return \Fhaculty\Graph\Walk + * @return Walk */ public static function factoryFromEdges(array $edges, Vertex $startVertex) { @@ -33,6 +37,117 @@ public static function factoryFromEdges(array $edges, Vertex $startVertex) return new self($vertices, $edges); } + /** + * create new cycle instance from given predecessor map + * + * @param Vertex[] $predecessors map of vid => predecessor vertex instance + * @param Vertex $vertex start vertex to search predecessors from + * @param int $by + * @param boolean $desc + * @return Walk + * @throws UnderflowException + * @see Edge::getFirst() for parameters $by and $desc + * @uses self::factoryFromVertices() + */ + public static function factoryCycleFromPredecessorMap($predecessors, $vertex, $by = Edge::ORDER_FIFO, $desc = false) + { + /*$checked = array(); + foreach ($predecessors as $vertex) { + $vid = $vertex->getId(); + if (!isset($checked[$vid])) { + + } + }*/ + + // find a vertex in the cycle + $vid = $vertex->getId(); + $startVertices = array(); + do { + $startVertices[$vid] = $vertex; + + $vertex = $predecessors[$vid]; + $vid = $vertex->getId(); + } while (!isset($startVertices[$vid])); + + // find negative cycle + $vid = $vertex->getId(); + // build array of vertices in cycle + $vertices = array(); + do { + // add new vertex to cycle + $vertices[$vid] = $vertex; + + // get predecessor of vertex + $vertex = $predecessors[$vid]; + $vid = $vertex->getId(); + // continue until we find a vertex that's already in the circle (i.e. circle is closed) + } while (!isset($vertices[$vid])); + + // reverse cycle, because cycle is actually built in opposite direction due to checking predecessors + $vertices = array_reverse($vertices, true); + + return self::factoryCycleFromVertices($vertices, $by, $desc); + } + + /** + * create new cycle instance with edges between given vertices + * + * @param Vertex[] $vertices + * @param int $by + * @param boolean $desc + * @return Walk + * @throws UnderflowException if no vertices were given + * @see Edge::getFirst() for parameters $by and $desc + */ + public static function factoryCycleFromVertices($vertices, $by = Edge::ORDER_FIFO, $desc = false) + { + $edges = array(); + $first = NULL; + $last = NULL; + foreach ($vertices as $vertex) { + // skip first vertex as last is unknown + if ($first === NULL) { + $first = $vertex; + } else { + // pick edge between last vertex and this vertex + $edges []= Edge::getFirst($last->getEdgesTo($vertex), $by, $desc); + } + $last = $vertex; + } + if ($last === NULL) { + throw new UnderflowException('No vertices given'); + } + + // additional edge from last vertex to first vertex + if ($last !== $first) { + $edges []= Edge::getFirst($last->getEdgesTo($first), $by, $desc); + $vertices []= $first; + } + + return new self($vertices, $edges); + } + + /** + * create new cycle instance with vertices connected by given edges + * + * @param Edge[] $edges + * @param Vertex $startVertex + * @return Walk + * @throws InvalidArgumentException if the given array of edges does not represent a valid cycle + * @uses self::factoryFromEdges() + */ + public static function factoryCycleFromEdges(array $edges, Vertex $startVertex) + { + $cycle = self::factoryFromEdges($edges, $startVertex); + + // ensure this walk is actually a cycle by checking start = end + if ($cycle->getVertexTarget() !== $startVertex) { + throw new InvalidArgumentException('The given array of edges does not represent a cycle'); + } + + return $cycle; + } + protected function __construct(array $vertices, array $edges) { $this->vertices = $vertices; @@ -207,7 +322,7 @@ public function isValid() foreach ($this->vertices as $vertex) { $vid = $vertex->getId(); // make sure vertex ID exists and has not been replaced - if (!isset($vertices[$vid]) || $vertices[$id] !== $vertex) { + if (!isset($vertices[$vid]) || $vertices[$vid] !== $vertex) { return false; } } diff --git a/tests/Fhaculty/Graph/Exception/NegativeCycleExceptionTest.php b/tests/Fhaculty/Graph/Exception/NegativeCycleExceptionTest.php index 85e237bd..0ef45305 100644 --- a/tests/Fhaculty/Graph/Exception/NegativeCycleExceptionTest.php +++ b/tests/Fhaculty/Graph/Exception/NegativeCycleExceptionTest.php @@ -1,14 +1,13 @@ getMockBuilder('Fhaculty\Graph\Cycle') + $cycle = $this->getMockBuilder('Fhaculty\Graph\Walk') ->disableOriginalConstructor() ->getMock(); diff --git a/tests/Fhaculty/Graph/WalkTest.php b/tests/Fhaculty/Graph/WalkTest.php new file mode 100644 index 00000000..ca3adbb6 --- /dev/null +++ b/tests/Fhaculty/Graph/WalkTest.php @@ -0,0 +1,144 @@ +createVertex(1); + $v2 = $graph->createVertex(2); + $v3 = $graph->createVertex(3); + $e1 = $v1->createEdgeTo($v2); + $e2 = $v2->createEdgeTo($v3); + + $walk = Walk::factoryFromEdges(array($e1, $e2), $v1); + + $this->assertEquals(3, $walk->getNumberOfVertices()); + $this->assertEquals(2, $walk->getNumberOfEdges()); + $this->assertSame($v1, $walk->getVertexSource()); + $this->assertSame($v3, $walk->getVertexTarget()); + $this->assertSame(array($v1, $e1, $v2, $e2, $v3), $walk->getAlternatingSequence()); + $this->assertTrue($walk->isValid()); + + $graphClone = $walk->createGraph(); + $this->assertGraphEquals($graph, $graphClone); + + return $walk; + } + + /** + * @param Walk $walk + * @depends testWalkPath + */ + public function testWalkPathInvalidateByDestroyingVertex(Walk $walk) + { + // delete v3 + $walk->getVertexTarget()->destroy(); + + $this->assertFalse($walk->isValid()); + } + + public function testWalkWithinGraph() + { + // 1 -- 2 -- 3 + $graph = new Graph(); + $v1 = $graph->createVertex(1); + $v2 = $graph->createVertex(2); + $v3 = $graph->createVertex(3); + $e1 = $v1->createEdgeTo($v2); + $e2 = $v2->createEdgeTo($v3); + + // construct partial walk "1 -- 2" + $walk = Walk::factoryFromEdges(array($e1), $v1); + + $this->assertEquals(2, $walk->getNumberOfVertices()); + $this->assertEquals(1, $walk->getNumberOfEdges()); + $this->assertSame($v1, $walk->getVertexSource()); + $this->assertSame($v2, $walk->getVertexTarget()); + $this->assertSame(array($v1, $e1, $v2), $walk->getAlternatingSequence()); + $this->assertTrue($walk->isValid()); + + $graphExpected = new Graph(); + $graphExpected->createVertex(1)->createEdgeTo($graphExpected->createVertex(2)); + + $this->assertGraphEquals($graphExpected, $walk->createGraph()); + + return $walk; + } + + public function testWalkLoop() + { + // 1 -- 1 + $graph = new Graph(); + $v1 = $graph->createVertex(1); + $e1 = $v1->createEdge($v1); + + $walk = Walk::factoryFromEdges(array($e1), $v1); + + $this->assertEquals(2, $walk->getNumberOfVertices()); + $this->assertEquals(1, $walk->getNumberOfEdges()); + $this->assertSame($v1, $walk->getVertexSource()); + $this->assertSame($v1, $walk->getVertexTarget()); + $this->assertTrue($walk->isValid()); + + return $walk; + } + + /** + * @param Walk $walk + * @depends testWalkLoop + */ + public function testWalkInvalidByDestroyingEdge(Walk $walk) + { + // destroy first edge found + foreach ($walk->getEdges() as $edge) { + $edge->destroy(); + break; + } + + $this->assertFalse($walk->isValid()); + } + + public function testWalkLoopCycle() + { + // 1 -- 1 + $graph = new Graph(); + $v1 = $graph->createVertex(1); + $e1 = $v1->createEdge($v1); + + $walk = Walk::factoryCycleFromEdges(array($e1), $v1); + + $this->assertEquals(2, $walk->getNumberOfVertices()); + $this->assertEquals(1, $walk->getNumberOfEdges()); + $this->assertSame($v1, $walk->getVertexSource()); + $this->assertSame($v1, $walk->getVertexTarget()); + $this->assertTrue($walk->isValid()); + } + + /** + * @expectedException InvalidArgumentException + */ + public function testWalkCycleInvalid() + { + // 1 -- 2 + $graph = new Graph(); + $v1 = $graph->createVertex(1); + $v2 = $graph->createVertex(2); + $e1 = $v1->createEdge($v2); + + Walk::factoryCycleFromEdges(array($e1), $v1); + } +}