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

Support attaching arbitrary data to vertices and edges #4

Closed
clue opened this issue Feb 25, 2013 · 14 comments
Closed

Support attaching arbitrary data to vertices and edges #4

clue opened this issue Feb 25, 2013 · 14 comments

Comments

@clue
Copy link
Member

clue commented Feb 25, 2013

We should support attaching arbitrary data to vertices and edges.

Implementing a setDataKey($key, $value) and getDataKey($key) is actually quite easy. But things to be considered:

  • Maybe implement ArrayAccess so that data can easily be set as $vertex[$key] = $value. While this may be appealing at first, how should we iterate over data set? Implementing Iterator to support foreach ($vertexOrEdge as $key => $value) { ... } should be okay, but it's a bit ambiguous.
  • Consider what to do when drawing the graph, importing and exporting graph files. For the mean time perhaps just ignore any additional data? But in the long term...?
@clemens-tolboom
Copy link
Collaborator

I prefer a

$o->setData($blob)

as my tool generates either a n-dimensional array or an object as it's data like

$data = array(
  'title' => (string),
  'content' => (object),
  'background-color => '#ffff00',
);

But there is a difference between data attached to ie a vertice itself like weight and ie formatters using graph.

See ie http://skillcompass.org which uses title and content differently.

@clemens-tolboom
Copy link
Collaborator

We need to add data to the graph too. Or should that be added to the formatter?

The image is done by http://drupal.org/project/graph_phyz on a 1024x800 canvas but the colors are vertice dependent. (module enabled,disabled, not available)

Graph Phyz colors

@clue
Copy link
Member Author

clue commented Feb 25, 2013

The data you mentioned above seems to be only focused on layouting, right? Setting layout attributes is one of the areas needing documentation, but this can be accomplished with setLayout($array) or setLayoutAttribute($key, $value):

$vertex->setLayout(array(
    'label' => 'vertex label',
    'tooltip' => 'Will be shown when hovering this vertex',
    'fillcolor' => '#ffff00',
));

See the GraphViz documentation for a list of valid attributes.
While this has been working quite good, its API is quite a bit of a mess...

Could you think of any other use cases?

@clemens-tolboom
Copy link
Collaborator

Tnx for the code example. I somehow expected the layout data into the GraphViz class. But either of the locations are 'bad'. Ie background-color (the css version) is not usable for GraphViz but probably usefull GraphML or Graph Phyz (html-divs). But let's not hijack this issue which is about vertice data.

In Drupal 8 there was a need for a Topologic Sorted List of JS Files so they needed to add the internal objects to vertices like

$asset = new Asset('file path');
$graph->addNode(spl_object_hash($asset), $asset);

which translates into

$asset = new Asset('file path');
$v = $g->createVertex(spl_object_hash($asset));
$v->setDataKey('asset', $asset);

@clue
Copy link
Member Author

clue commented Feb 26, 2013

Just scanned through the related tickets, interesting approach...

Though the related code can be worked around with an additional external container, it's probably more convenient adding the data to the vertex instances:

$assets[++$assetId] = new Assert($path);
$g->createVertex($assetId); /* create vertex with the same temporary asset Id for later reference */

$vertices = callMethodToDoTopologicalSortingAndReturnOrderedVertices($g);
foreach ($vertices as $vid => $ignoreVertex) {
    $asset = $assets[$vid];
}

vs.

$asset = new Asset($path);
$v = $g->createVertex(); /* vertex ID is unused anyway */
$v->setDataKey('asset', $asset);

$vertices = callMethodToDoTopologicalSortingAndReturnOrderedVertices($g);
foreach ($vertices as $vertex) {
    $asset = $vertex->getDataKey('asset');
}

@clue
Copy link
Member Author

clue commented Nov 19, 2013

I'm tempted to close this one in favor of #26. Adding a general purpose data attribute will result in a ambiguous definition, so I very much prefer recommending to extend the base Vertex class instead.

Any objections?

@clemens-tolboom
Copy link
Collaborator

My use case is generating the graph from Drupal CMS injecting data to render later on into a json data structure for use in ie http://d3js.org/

I don't want to extend stuff for this as that would mean extending graph.php. That makes it harder to implement my CMS stuff :p

Can't we have a similar approach we did for toString() implementing a default property bag?

@clue
Copy link
Member Author

clue commented Dec 25, 2013

[…] into a json data structure for use in ie http://d3js.org/

See my recent comments in PR #40. Using d3 for layouting is currently(!) out of scope of this project, and as such there's no immediate need for "custom data attributes".

Adding these attributes will have no effect for GraphViz layouting anyway, hence the suggestion to extend the base Vertex class for your use-case instead.

Perhaps we can come up with a solution that fits both GraphViz and d3?

@clemens-tolboom
Copy link
Collaborator

This issue seems like a won't fix to me now you're working on #40 as the arbitrary data is what I means for use with thejit or d3js and even my graph_phyz

@clue
Copy link
Member Author

clue commented Dec 30, 2013

This issue seems like a won't fix to me […]

Kind of :) It's not that I don't want to, but more that I don't see a valid use case that does not violate any or our constraints. PR #40 addresses our layouting needs and #82 adds support for extending the base classes for a much cleaner design.

Also, the following code works just fine:

$v = $graph->createVertex();
$v->myCustomAttribute = 'hello world';
$v->myBlob = array();

As such, I'm more tempted to close this as either "duplicate" or "invalid".

@clemens-tolboom
Copy link
Collaborator

Closing as 'invalid' :p

@clue
Copy link
Member Author

clue commented Dec 30, 2013

Alright :) Thanks for the valuable discussion!

@clue
Copy link
Member Author

clue commented Mar 22, 2014

Just for the reference, the initial idea of this ticket is now being tracked in #100.

@clemens-tolboom
Copy link
Collaborator

Thanks for updating :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants