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 undirected #92

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dionyziz
Copy link

@dionyziz dionyziz commented Aug 25, 2018

This patch changes the behavior of undirected graph functions inEdges and outEdges to return all the edges. This is in accordance to the relevant mathematical literature where, typically, when discussing "out edges" or "in edges" of undirected graphs, we mean all the edges.

Because the Dijkstra shortest path algorithm uses the outEdges function, it did not always work on undirected graphs. The behavior worked only when the natural order of the edges matched the shortest path traversal order of the edges; otherwise incorrect (such as Infinity) path lengths would be returned.

This patch adds a regression test for this edge case.

Merging this patch will require updating the documentation, which I will do as soon as it is merged.

Fix #42

@BudgieInWA
Copy link

As nodeEdges, inEdges and outEdges all exists for undirected graphs, what if outEdges guaranteed that for each returned edge e, e.v === v and inEdges gauranteed that e.w === v. This is a condition that is always true for directed graphs, and it is a condition that one might assume is always true by the name of the method. It is also a condition that is very useful, for example it would mean that the ternary on dijkstra.js#24 would not be needed (if the edgeFn argument didn't exist).

nodeEdges could guarantee that edges are returned in a consistent orientation, which would be convenient when edges need to be processed just once.

@lutzroeder lutzroeder force-pushed the master branch 3 times, most recently from 64a1b87 to cf48a25 Compare February 16, 2019 02:57
@assafsun
Copy link

Hi @dionyziz

I'm trying to review the open PR in order to reduce the amount of issues and PRs.
I reviewed your PR, nice work!
Two main item that I saw are:

  1. merge action of lodash was not defined.
  2. The nodeedges function extraction result in test failures.

I took your changes and kept a simpler flow based on your changes and the tests )With your new test) pass. the undirect graph will use the entire node edges and just the in or out.

You can see my changes in the PR that I will create

In case you are you still here and want to progress your work, feel free to comment and review the changes :)

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.

Dijkstra distance between nodes are Infinity when using undirected graph
3 participants