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.

Sign up for GitHub

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 nested vertex groups (subgraphs) #36

Closed
wants to merge 7 commits into from
Closed

[WIP] Support nested vertex groups (subgraphs) #36

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
be85afb
Setting up test and fixtures for subgraphs.
clemens-tolboom Jun 1, 2013
19ce548
Added GroupTest.
clemens-tolboom Jun 1, 2013
190eeba
Add NULL for no group.
clemens-tolboom Jun 4, 2013
7a0d0fd
Allow for string as Group ID.
clemens-tolboom Jun 4, 2013
1ca74f0
Add string groupIDs to tests.
clemens-tolboom Jun 4, 2013
986643f
Make Graphviz cluster recursive.
clemens-tolboom Jun 4, 2013
8dee7da
Fixed merge conflict.
clemens-tolboom Mar 3, 2014
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
4 changes: 3 additions & 1 deletion lib/Fhaculty/Graph/Algorithm/Groups.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ public function getGroups()
{
$groups = array();
foreach ($this->graph->getVertices() as $vertex) {
$groups[$vertex->getGroup()] = true;
$group = $vertex->getGroup();
$group = (is_null($group)? "_NULL_" : $group);
Copy link
Member

Choose a reason for hiding this comment

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

Minor nitpick regarding code-style:

if ($group === null) {
    $group = '_NULL_';
}

Also, if the default group is null, it should come out as such (if at all possible). We also have to make sure to handle the case when a group with the literal name "_NULL_" does already exist. We should not merge them with the vertices with no group at all (null).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When having no group the vertex is not part of any group :)

_NULL_ is arbitrary but the group IDs must follow a pattern A:B:C that is using a :

Copy link
Member

Choose a reason for hiding this comment

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

$graph = new Graph();
$graph->createVertex(1);
$graph->createVertex(2)->setGroup('A');

$alg = new Groups($graph);
var_dump($alg->getNumberOfGroups());
var_dump($alg->getGroups());

As a user, I'd expect to receive:

(int)2
array(2) {
  [0]=>
  NULL
  [1]=>
  string(1) "A"
}

I guess we may a different understanding here?

[edit] fixed invalid format for var_dump()

$groups[$group] = true;
}

return array_keys($groups);
Expand Down
68 changes: 44 additions & 24 deletions lib/Fhaculty/Graph/GraphViz.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,33 +299,35 @@ public function createScript()
$gid = 0;
$indent = str_repeat($this->formatIndent, 2);
// put each group of vertices in a separate subgraph cluster
foreach ($alg->getGroups() as $group) {
$script .= $this->formatIndent . 'subgraph cluster_' . $gid++ . ' {' . self::EOL .
$indent . 'label = ' . $this->escape($group) . self::EOL;
foreach($alg->getVerticesGroup($group) as $vid => $vertex) {
$layout = $this->getLayoutVertex($vertex);

$script .= $indent . $this->escapeId($vid);
if($layout){
$script .= ' ' . $this->escapeAttributes($layout);
}
$script .= self::EOL;
$groups = $alg->getGroups();
$tree = array();
foreach ($groups as $group) {
$fragments = explode(":", $group);
Copy link
Member

Choose a reason for hiding this comment

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

We should probably add a configuration option to change the separation character.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would need an alien GraphViz setting (that is not in http://www.graphviz.org/content/attrs) for group separator as Graphviz.php produced a new Group();
(/me puzzeled)

Copy link
Member

Choose a reason for hiding this comment

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

Currently, there's no way to set GraphViz::$formatIndent either. I think it's okay if we introduce a $groupSeparator in a similar fashion. Just trying to make sure to not hard-code any magic strings :)

$pointer = &$tree;
while (count($fragments)) {
$key = array_shift($fragments);
if (!isset($pointer[$key])) {
$pointer[$key] = array();
}
$script .= ' }' . self::EOL;
$pointer = &$pointer[$key];
}
Copy link
Member

Choose a reason for hiding this comment

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

Really interesting concept! 👍

Perhaps this should be moved to the Algorithm\Groups instead? Also some comments and/or unit tests would be nice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean? The Cluster concept? That would indeed be nice.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, perhaps a Groups::getClusteredGroups($separator = ':') or whatever name you come up with 👍 Sounds useful and can probably be reused in other situation as well :)

}
} else {
// explicitly add all isolated vertices (vertices with no edges) and vertices with special layout set
// other vertices wil be added automatically due to below edge definitions
foreach ($this->graph->getVertices() as $vid => $vertex){
$layout = $this->getLayoutVertex($vertex);

if($vertex->isIsolated() || $layout){
$script .= $this->formatIndent . $this->escapeId($vid);
if($layout){
$script .= ' ' . $this->escapeAttributes($layout);
}
$script .= self::EOL;
foreach( $tree as $group => $subtree) {
$this->printCluster($group, $subtree, $alg, $script);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I suppose we still need this block.. Otherwise the vertices end up being listed twice: once in their subgraph and once in the main graph.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope.

Vertices must be listed at full after the clustering which only have their IDs listed. But this is WIP as I have to add a real layout test still :(

Copy link
Member

Choose a reason for hiding this comment

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

I'd love to see some insight onto why this is a must. Perhaps this will be clear once we have those layout tests 👍 If possible I'd like the output to be as concise as possible and not contain any duplication.


// explicitly add all isolated vertices (vertices with no edges) and vertices with special layout set
// other vertices wil be added automatically due to below edge definitions
foreach ($this->graph->getVertices() as $vid => $vertex){
$layout = $this->getLayoutVertex($vertex);

if($vertex->isIsolated() || $layout){
$script .= $this->formatIndent . $this->escapeId($vid);
if($layout){
$script .= ' ' . $this->escapeAttributes($layout);
}
$script .= self::EOL;
}
}

Expand Down Expand Up @@ -356,6 +358,24 @@ public function createScript()
return $script;
}

protected function printCluster($group, $tree, $alg, &$script, $depth = 1) {
if ($group == "_NULL_") {
return;
}
$script .= str_repeat($this->formatIndent, $depth) . 'subgraph cluster_' . str_replace(":", "_", $group) . ' {' . self::EOL;
Copy link
Member

Choose a reason for hiding this comment

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

$group could possibly contain unsafe characters, now that every string input is allowed, so it should be escaped. Also, we have to make sure to not cause any clashes when renaming "a:1" to "a_1" because "a_1" could already exist.

IMO however, it can be removed completely. The old way was to use just incrementing numbers in order to avoid clashes. The actual group name is added as a subgraph label anyway, so I don't think the real subgraph name is even needed?

Any thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point about clashes. But is a_1 somehow related to cluster_a_1?

We do have (or at least should add) a constaint on group IDs

Copy link
Member

Choose a reason for hiding this comment

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

The previous constraint was forcing it to be an integer 😉 So whatever we can come up with, we've already improved that situation.

If possible however, I'd like to not enforce any constraints. As far as I can tell this should not be an issue: The subgraph name will only be used in the dot file and will not be shown in the final image file, so it should probably be okay to use an arbitrary/random identifier and make sure to add the real group ID as a subgraph label (which is already done one line below).

$script .= str_repeat($this->formatIndent, $depth + 1) . 'label = ' . $this->escape($group) . self::EOL;
foreach($alg->getVerticesGroup($group) as $vid => $vertex) {
$script .= str_repeat($this->formatIndent, $depth + 1) . $this->escape($vid);
Copy link
Member

Choose a reason for hiding this comment

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

Indentation should probably not be evaluated for every single line :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True ... will fix that later on.

$script .= self::EOL;
}
if (!empty($tree)) {
foreach($tree as $sub_group => $sub_tree) {
$this->printCluster($group . ':' . $sub_group, $sub_tree, $alg, $script, $depth + 1);
}
}
$script .= str_repeat($this->formatIndent, $depth) . '}' . self::EOL;
}

/**
* escape given id string and wrap in quotes if needed
*
Expand Down
12 changes: 4 additions & 8 deletions lib/Fhaculty/Graph/Vertex.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,10 +218,10 @@ public static function getAll($vertices, $by = self::ORDER_FIFO, $desc = false)
/**
* group number
*
* @var int
* @var int|string
* @see Vertex::setGroup()
*/
private $group = 0;
private $group = NULL;

/**
* Creates a Vertex (MUST NOT BE CALLED MANUALLY!)
Expand Down Expand Up @@ -301,15 +301,11 @@ public function getFlow()
/**
* set group number of this vertex
*
* @param int $group
* @param int | string $group
* @return Vertex $this (chainable)
* @throws InvalidArgumentException if group is not numeric
*/
public function setGroup($group)
{
if (!is_int($group)) {
throw new InvalidArgumentException('Invalid group number');
}
$this->group = $group;

return $this;
Expand All @@ -318,7 +314,7 @@ public function setGroup($group)
/**
* get group number
*
* @return int
* @return int| string
*/
public function getGroup()
{
Expand Down
31 changes: 31 additions & 0 deletions tests/Fhaculty/Graph/Algorithm/GroupTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php

use Fhaculty\Graph\Algorithm\Groups as Groups;
use Fhaculty\Graph\Graph;

class GroupTest extends TestCase
{
public function testGroupsTwo()
{
$graph = new Graph();
$graph->createVertex('no group');
$graph->createVertex('group 0')->setGroup(0);

$alg = new Groups($graph);
$this->assertEquals(2, $alg->getNumberOfGroups(), 'Empty group must differ from 0');

}

public function testGroupsOneAndTwo()
{

$graph = new Graph();
$graph->createVertex('group 1')->setGroup(1);
$graph->createVertex('group 2.1')->setGroup(2);
$graph->createVertex('group 2.2')->setGroup(2);

$alg = new Groups($graph);
$this->assertEquals(2, $alg->getNumberOfGroups());

}
}
32 changes: 32 additions & 0 deletions tests/Fhaculty/Graph/GraphVizTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,38 @@ public function testEdgeLabels()
$this->assertEquals($expected, $this->getDotScriptForGraph($graph));
}

public function testSubgraph()
{
$graph = new Graph();
$graph->createVertex('n1')->setGroup("A");
$graph->createVertex('n2')->setGroup("B");
$graph->createVertex('n21')->setGroup("B:A");
$graph->createVertex('n22')->setGroup("B:A");

$expected = file_get_contents(__DIR__ . '/fixtures/graph-cluster.dot');

$this->assertEquals($expected, $this->getDotScriptForGraph($graph));

}

public function testSubgraphTree()
{
$graph = new Graph();
$graph->createVertex('n1')->setGroup('A');
$graph->createVertex('n21')->setGroup("B");
$graph->createVertex('n211')->setGroup('B:A');
$graph->createVertex('n212')->setGroup('B:A');
$graph->createVertex('n2111')->setGroup('B:A:A');
$graph->createVertex('n3');

$in_file = __DIR__ . '/fixtures/graph-cluster-tree.dot';
$out_file = __DIR__ . '/fixtures/out/graph-cluster-tree.dot';
$expected = file_get_contents($in_file);
file_put_contents($out_file, $this->getDotScriptForGraph($graph));

$this->assertEquals($expected, $this->getDotScriptForGraph($graph));

}

private function getDotScriptForGraph(Graph $graph)
{
Expand Down
26 changes: 26 additions & 0 deletions tests/Fhaculty/Graph/fixtures/graph-cluster-complex-tree.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Graph has a name (tooltip in SVG output)
graph Faculty {
// cluster_ is a special prefix making it a box with a label
subgraph cluster_graph {
label = "<<package>>\nGraph"

graph_php [
label = "Graph.php"
]

// Note the absence of cluster_ making their groupness hard to tell
subgraph algorithm {
label = "<<package>>\nAlgorithm"
subgraph cluster_maxflow {
label = "<<package>>\nMaxFlow"
EdmondsKarp
}
AlgorithmX -- AlgorithmY
}
subgraph cluster_edge {
label = "<<package>>\nEdge"
P
Q
}
}
}
25 changes: 25 additions & 0 deletions tests/Fhaculty/Graph/fixtures/graph-cluster-tree.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
graph G {
subgraph cluster_A {
label = "A"
"n1"
}
subgraph cluster_B {
label = "B"
"n21"
subgraph cluster_B_A {
label = "B:A"
"n211"
"n212"
subgraph cluster_B_A_A {
label = "B:A:A"
"n2111"
}
}
}
"n1"
"n21"
"n211"
"n212"
"n2111"
"n3"
}
19 changes: 19 additions & 0 deletions tests/Fhaculty/Graph/fixtures/graph-cluster.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
graph G {
subgraph cluster_A {
label = "A"
"n1"
}
subgraph cluster_B {
label = "B"
"n2"
subgraph cluster_B_A {
label = "B:A"
"n21"
"n22"
}
}
"n1"
"n2"
"n21"
"n22"
}
1 change: 1 addition & 0 deletions tests/Fhaculty/Graph/fixtures/out/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This directory is used to generate output from some fixtures for visual inspection.
25 changes: 25 additions & 0 deletions tests/Fhaculty/Graph/fixtures/out/graph-cluster-tree.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
graph G {
subgraph cluster_A {
label = "A"
"n1"
}
subgraph cluster_B {
label = "B"
"n21"
subgraph cluster_B_A {
label = "B:A"
"n211"
"n212"
subgraph cluster_B_A_A {
label = "B:A:A"
"n2111"
}
}
}
"n1"
"n21"
"n211"
"n212"
"n2111"
"n3"
}
51 changes: 51 additions & 0 deletions tests/Fhaculty/Graph/fixtures/out/in/graph-cluster-tree.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.