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

Line graph algorithm doesn't handle self-loops correctly #937

Merged
merged 4 commits into from
Sep 9, 2013

Conversation

chebee7i
Copy link
Member

The algorithm in line_graph() doesn't capture self-loops correctly.
See the discussion and code examples at
http://stackoverflow.com/questions/18214305/correctness-of-networkx-line-digraph-construction/18244242

@chebee7i
Copy link
Member

I can take a look at this later if no one jumps on it before then. I agree that the correct "answer" really depends on the type of construction you want. Here is a quick and dirty translation of the line graph construction I use in my own work with multidigraphs. If people like this construction, we can clean it up and make it handle all graph types too. In the output below, the new nodes are (from node, to node, key). For non-multi digraphs, the keys wouldn't be there.
nx_dqavzonx_zujbdl

@hagberg
Copy link
Member Author

hagberg commented Aug 16, 2013

I'd like to implement the standard definition for graphs and digraphs and reference the definition.
I'm not sure there is one for multigraphs?

@chebee7i
Copy link
Member

I don't think multi(di)graphs require changes to the definitions. The reason being that the definitions are defined on edges---having additional edges will not change that definition any more than would adding non-multiedges to the same graph. So each edge in a multi(di)graph has a key associated with it, but that key is ignored when determining edge adjacency.

Interestingly, it seems that "standard" definition for directed graphs is not the obvious analogous definition from undirected graphs. The main difference being that with undirected graphs, you pop the current node (an edge in G) from the list of all nodes and then you connect any nodes which represent incident edges. For directed graphs, the standard definition (e.g. that used for de Bruijin graphs) is that you do not pop the current node.

Consider a multigraph with the following edges: (A,A), (A,B), (A,B).
Its line graph has the following edges: (AA, AB0), (AA, AB1)

Consider a multidigraph with the following edges: (A,A), (A,B), (A,B)
Its line graph has the following edges: (AA, AA), (AA, AB0), (AA, AB1)
The point being that each of the 3 edges represents one of the 3 possible length-2 paths.

It seems to me that you could define line graphs for digraphs in an exactly analogous way (and that would correspond to the code you posted on SO). For some reason though, the emphasis with digraphs has been on the line graph representing all possible paths, and thus, it has deviated.

We could perhaps provide: line_graph() and line_digraph(). line_graph() pops the current edge when determining adjacency in the line graph, while line_digraph() does not. To allow people to use both definitions, we simply allow both functions to work with graphs and digraphs. So in principle, you could construct the line graph of a multidigraph, and also the line digraph of a multidigraph (and these would not be the same thing).

To make it clear what I am proposing:

edges: [(A,A), (A,B), (A,B)]

g is a MultiGraph with those edges.
line_graph(g)    is of class g.__class__ with edges:   (AA, AB0),  (AA, AB1)
line_digraph(g)  is of class g.__class__ with edges:   (AA, AA), (AA, AB0), (AA, AB1)

g is MultiDiGraph with those edges.
line_graph(g)    is of class g.__class__ with edges:   (AA, AB0),  (AA, AB1)
line_digraph(g)  is of class g.__class__ with edges:   (AA, AA), (AA, AB0), (AA, AB1)

@hagberg
Copy link
Member Author

hagberg commented Aug 24, 2013

Thanks @chebee7i for this very helpful discussion.

I vote for only one function with the standard definitions. That is, there one function that produces line graphs for directed and undirected graphs each with a different definition as found in the current literature.

Is there any ambiguity about what to do with self loops?

@ghost ghost assigned chebee7i Aug 24, 2013
@chebee7i
Copy link
Member

Great. I don't think there should be any ambiguity with self loops, but we'll see I suppose. I'll go ahead and give this a try with plenty of tests so at least its clear that the functions are doing what they intend. Once I get code, I'll try attach the pull request to this github issue (http://stackoverflow.com/questions/4528869/how-do-you-attach-a-new-pull-request-to-an-existing-issue-on-github).

@chebee7i
Copy link
Member

I had to define some auxiliary functions to hide some of the API differences between multi-and non-multi (di)graphs, but the result is that the main construction code is fairly readable and intuitive (I hope).

@hagberg
Copy link
Member Author

hagberg commented Sep 8, 2013

This PR looks good to me. I especially like the thorough job documenting the graph/digraph issue.

Here are a few comments

  • We generally don't require nodes or edges to be sortable. I see in the original line graph algorithm that we violated that assumptoin. In this PR

@chebee7i
Copy link
Member

chebee7i commented Sep 8, 2013

Opps. Looks like rest of the comment got mangled. Repost.

@hagberg
Copy link
Member Author

hagberg commented Sep 8, 2013

Ugh- something strange with Github. It got mangled once before an no time for long note... The points were

  • Either make a note that you must use sortable nodes or fix to work without sort
  • Make lg_undirected and lg_directed part of the namespace (with better names) or prefix with underscore
  • edges and node functions get underscore and node func maybe private to fcn
  • add yourself as author

Add comment about line graph requiring sortable nodes in G.
chebee7i added a commit that referenced this pull request Sep 9, 2013
Update line graph algorithm to handle self-loops correctly
@chebee7i chebee7i merged commit 3b528b5 into networkx:master Sep 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants