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

Dijkstra - support undirected graphs #115

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

assafsun
Copy link

Dijkstra - support undirected graphs

issue #42

@assafsun
Copy link
Author

The changes are based on the work of PR after rebase, review and running the tests
#92

Comment on lines +505 to +507
var inEdges = this.getInEdges(v, w, false);
if (inEdges) {
return inEdges.concat(this.outEdges(v, w));
return inEdges.concat(this.getOutEdges(v, w, false));

Choose a reason for hiding this comment

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

As far as I can tell, getInEdges only has two arguments, v and optionally w, making the false seem useless here.

@BudgieInWA
Copy link

Currently inEdges and outEdges are undefined for undirected graphs with the documentation recommending nodeEdges instead. Instead of having outEdges behave exactly like nodeEdges, I think it would be useful for outEdges(v) to guarantee that it returns edges oriented away from v, by reversing those oriented towards v (and similarly for inEdges). outEdges behaving exactly like nodeEdges is a convenience at best and I argue that it is confusing and likely to lead to incorrect usage.

With the existing API, an algorithm that wishes to traverse both directed and undirected graphs in the normal way (respecting the direction of edges in directed graphs but not in undirected graphs) might do something like this:

function visit(v) {
  let edges;
  // Let's not do any more work than we need to:
  if (g.isDirected()) {
    edges = g.outEdges(v);
  } else {
    function orientAwayFromV(e) { ... }
    edges = g.nodeEdges(v).map(orientEdgeAwayFromV);
  }
  // For the rest of this function edges are oriented in the direction
  // that we are traversing them so it doesn't matter if the graph is
  // directed or not.

  for (const e of edges) {
    if (shouldTraverse(e)) {
       console.log("traversing edge from", e.v, "to", e.w);
       visit(e.w);
    }
  }
}

The new API implemented in this PR cannot help us here if we want to avoid unnecessarily checking edge orientation. Also, if the documentation said that outEdges works on directed graphs, const edges = g.outEdges(v); would look correct, but some edges would be backwards.

I propose implementing outEdges something like this:

Graph.prototype.outEdges = function(v, w) {
  return this.isDirected()
    ? this.getOutEdges(v, w)
    : this.getOutEdges(v, w).concat(reverseEdges(this.getInEdges(v, w));
};

That way, I could write my example like this supporting directed and undirected graphs without having to manually check edge direction:

function visit(v) {
  const edges = g.outEdges(v);
  
  for (const e of edges) {
    if (shouldTraverse(e)) {
       console.log("traversing edge from", e.v, "to", e.w);
       visit(e.w);
    }
  }
}

@davidepaci
Copy link

If anyone else is having trouble with this please refer to this answer

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

Successfully merging this pull request may close these issues.

3 participants