[WIP] Support nested vertex groups (subgraphs)

[clemens-tolboom]

This pull request will not make it yet.

It addresses several failures:

• sub graphs are not well supported
  • it's a one shot either all belong to a subgraph or non
  • there is no option for sub graphs containing subgraphs
• the group ID is used for the subgraph label
• Group ID is numeric. Why so?
• How can we make a hierarchical structure.

The branch contains a new directory containing fixtures. This is mimicked from https://github.com/symfony/symfony/tree/master/src/Symfony/Component/Translation/Tests

By using fixtures we are able to generate the visual outputs as well (not yet implemented)

The 2 new generate files as output into fixtures/out ... I'm not sure that is the best way but it will let us generated an SVG from the 'real' output.

Please tell how we can address these several issues :)

[clemens-tolboom]

I've added a GroupTest as no group === setGroup(0) is bad.

As an extended question to "groupID is numeric" it think we can make it a String and then can address hierarchical by define allowed values structured as path like a/b/c which then defines it's hierarchical nature. I don't think that's the best solution and it'll probably break the meaning of Groups.php.

[clue]

I agree that the current situation is a bit limited and cumbersome to work with. And your tests highlight those limitations very well 👍

Simply put, the current implementation was easiest to start with and did just what was needed to implement the Algorithm\Bipartit. As such, it's not very well designed.

Instead of extending Vertex::setGroup() however, I'm in favor of adding a new Subgraph class. This way, we will be able to define algorithms and set layout attributes for subgraphs.

[clemens-tolboom]

The latest commits makes group string or int and missing group is "_NULL_"

The rendering of Graphviz format does render the clusters including IDs of containing vertices. Next prints all vertice including their layout.

[clue]

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).

[clemens-tolboom]

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 :

[clue]

$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()

[clue]

Now with all my (minor) remarks in place, let me emphasize one thing: thanks for the high quality PR! 👍
I'm really interested in seeing this merged once the remaining tasks (I don't even dare calling them "issues") have been resolved.

[clemens-tolboom]

I'm not so sure a cluster === vertex as http://www.google.nl/search?q=graphviz+subgraph (images) does not give me any cluster object-ness.

@clue I'll create a new issue for you (hint) to try adding cluster to Graph.php

[clue]

I've commented on the related ticket and I'm very positive about the idea. But afaikt this is not necessarily required for this ticket.

Imo, as a first step, we should finish this ticket with the proposed changes and then at a later stage try to introduce some kind of OOP handling for groups/subgraphs (or whatever name we come up with).

[clemens-tolboom]

This is quite old :( ... but I ran into a sub graph related puzzle regarding http://www.graphviz.org/content/node-shapes#record and it's port positions

A vertex label can have different edge snap points defined by the fields defined in the label.

This patch needs a re-roll ... not sure whether that will work.

[clue]

not sure whether that will work

I don't think these features are related at all, so it certainly makes sense to track port definitions in your separate ticket (#92).

This patch needs a re-roll

I would certainly appreciate that :)

[clemens-tolboom]

I tried to rebase but failed. Please help :)

Both $vertex->isIsolated() and $vertex->isVertexIsolated() seemed gone.

@clue can you try to rebase ... next I can work on the issues mentioned.

[clue]

I tried to rebase but failed.

Care to share your results? I'll put this on my TODO list, but expect this to take a while.

Both $vertex->isIsolated() and $vertex->isVertexIsolated() seemed gone.

Yes, they've been moved with v0.6.0, see changelog.

[clemens-tolboom]

I'm not sure why this fails

There was 1 failure:

1) VertexTest::testGroupInvalid

Failed asserting that exception of type "InvalidArgumentException" is thrown.

[clemens-tolboom]

Furthermore I pushed to origin pr/36 by accident. Did I break it? Not sure. Sorry for any inconveniences. Please remove me from the commit list as graphviz is moved to it's own repo

[clue]

1. VertexTest::testGroupInvalid

Without this PR we would only accept integer group IDs and hence throw an InvalidArgumentException for any other data type. Now that we're lifting this limit, we need to update the test suite (i.e. remove this test).

Furthermore I pushed to origin pr/36 by accident.

No worries, I've just removed that branch. I'm not sure how changing your permissions has an effect on your outstanding PRs, so I'll change it once they're resolved.

[clue]

Some recent updates on this repo:

• GraphViz has been split off via Split off GraphViz #115.
• Ticket Add general purpose Attributes, replace Layoutable #103 added general purpose attributes for each entity.
• The remaining vertex properties (group) will eventually be removed as per Use Attributes for all mathematical properties (weight, balance etc.) #114.

Ping @clemens-tolboom, would you care to give some status on this PR?

[clemens-tolboom]

No time left :-( I really hope to help again but not in the foreseeable future.

[clue]

No worries and thanks for your continuous support! 👍

I'll close this PR for now and referenced this in a new ticket in the graphviz component. Still looking forward to seeing this in, so let's see :)