Strongly connected components method addition #3

Merged
merged 13 commits into from Jan 13, 2017

Projects

None yet

2 participants

@luccitan
Contributor

Implementation of the path-based SCC algorithm.

There are two alternatives algorithms :

  • Kosaraju's algorithm : it's considered a simple and intuitive algorithm but also considered as less efficient in practice, compared to the path-based one and the next one
  • Tarjan's algorithm : another efficient algorithm (linear-time). But I think that the handling of bound properties to the vertices (cf. v.index, v.lowlink, etc... in the algorithm) would require data structures in Javascript that worsen the implementation complexity
@Yomguithereal
Member
  1. Can you also add the related documentation on README.md?
  2. Have you run the linter using npm run lint?
@luccitan
Contributor
  1. The README.md has been updated.
  2. The linting has been done. The only remaining error is a non-recognized Map().
test.js
-var connectedComponents = require('./');
+var connectedComponents = lib.connectedComponents,
+ stronglyConnectedComponents = lib.stronglyConnectedComponents;
@Yomguithereal
Yomguithereal Jan 11, 2017 Member

Indentation.

index.js
+ var nodes = graph.nodes();
+
+ if (!graph.size)
+ return nodes.map(function(node) { return [node]; });
@Yomguithereal
Yomguithereal Jan 11, 2017 Member

It would be preferable to use a straight loop rather than .map for performance reasons.

index.js
+ pop,
+ neighbOrder;
+
+ var DFS = function(node) {
@Yomguithereal
Yomguithereal Jan 11, 2017 Member

It's possible to perform a DFS using a stack as in the connectedComponents function to avoid using a function which can get costly if used recursively. But we can tackle this later.

index.js
+ P.push(node);
+ S.push(node);
+
+ graph.outboundNeighbors(node).forEach(function(neighbor) {
@Yomguithereal
Yomguithereal Jan 11, 2017 Member

Same remark. Avoid using .forEach. This is completely ok for application code but it usually hurts performance for low-level library code.

@luccitan
Contributor

Pushed e8e1cc7 to update from the review.

The DFS function has not been updated.

index.js
@@ -73,29 +73,39 @@ exports.stronglyConnectedComponents = function(graph) {
if (graph.type === 'undirected')
throw new Error('graphology-components: the given graph is undirected');
- var nodes = graph.nodes();
+ var nodes = graph.nodes(),
+ components = [],
@Yomguithereal
Yomguithereal Jan 11, 2017 Member

Tu as un problème d'indentation sur tes variables. Le components devrait être aligné avec nodes.

@Yomguithereal
Yomguithereal Jan 11, 2017 Member

Tu as ce problème un peu partout.

index.js
+ P.push(node);
+ S.push(node);
+
+ neighbors = graph.outboundNeighbors(node);
@Yomguithereal
Yomguithereal Jan 12, 2017 Member

Si j'ai bien compris notre discussion d'hier soir, on va considérer les arcs non-orientés comme des arcs mutuels. Or, la méthode outboundNeighbors ne fait pas tout à fait ça (elle ne va renvoyer qu'un subset des arcs non-orientés pour les itérations en ayant besoin). Donc, dans ton cas, je pense qu'il faut plutot faire l'union de outNeighbors et undirectedNeighbors genre:

neighbors = graph.outNeighbors(node).concat(graph.undirectedNeighbors(node));
@luccitan
luccitan Jan 12, 2017 Contributor

outNeighbors renvoie bizarrement cette erreur lors des tests :

graphology-components #.stronglyConnectedComponents should return the correct components. (mixed edges): NotFoundGraphError: Graph.outNeighbors: could not find the "undefined" node in the graph.

C'est étrange, surtout que les méthodes itératives sont générés dynamiquement donc sur le même 'schéma' de parcours des noeuds, etc...

@Yomguithereal
Yomguithereal Jan 12, 2017 Member

Là, l'erreur te dit que le node que tu as passé à la méthode n'existe pas dans le graph. Check la valeur de ta variable node.

@luccitan
luccitan Jan 12, 2017 edited Contributor

Pardon, problème de ma part avec le scope des variables avec les appels récursifs ... :-)

@luccitan
Contributor

Added an annotation for weakly connected components

The term is specific to a mixed or directed graph, but the algorithm is the same as for connected components : it ensures that there is a path between every two nodes of a component

@Yomguithereal
Member

I think we're all good now. I'll merge we possible. Thanks @luccitan.

+
+var sortComponents = function(components) {
+ components.forEach(function(c) {
+ c.sort(function(a, b) {
@Yomguithereal
Yomguithereal Jan 13, 2017 Member

En JavaScript, .sort mute l'array original, donc ta fonction n'a pas besoin de retourner components et peut se contenter de faire des effets de bord. Aussi, vérifie qu'une méthode d'assertion de chai ne ferait pas le taf à la place de ton sort ici.

@luccitan
luccitan Jan 13, 2017 Contributor

Il n'y a pas de méthodes satisfaisantes avec chai pour gérer le tri des tableaux imbriqués :|

@Yomguithereal Yomguithereal merged commit d78f74b into graphology:master Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment