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

Add mapping dict as graph attribute in condensation. #1141

Merged
merged 2 commits into from
May 11, 2014

Conversation

jtorrents
Copy link
Member

I've found myself several times needing a mapping of original nodes to condensation nodes (ie strongly connected components). It is easy to compute that myself (especially since condensation function accepts precomputed SCC as a parameter). But then the function itself recalculates the mapping for building the condensation graph. Adding the mapping as graph attribute is simple, backwards compatible, and avoids redundant computations.

I've also updated condensation docstrings and added a test.

Updated documentation and added a test.
@@ -334,4 +336,6 @@ def condensation(G, scc=None):
for u,v in G.edges():
if mapping[u] != mapping[v]:
C.add_edge(mapping[u],mapping[v])
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe some code maintenance can be piggybacked.

for i, component in enumerate(scc):
    mapping.update((n, i) for n in component)
number_of_components = i + 1
C.add_nodes_from(range(number_of_components))
C.add_edges_from((u, v) for u, v in G.edges_iter()
                 if mapping[u] != mapping[v])

@ysitu
Copy link
Contributor

ysitu commented May 10, 2014

LGTM. But exposing the computed SCCs directly is also a convenience to the user:

for i, component in enumerate(scc):
    C.node[i]['members'] = list(scc)

Also improved the code for condensation by avoiding unnecessary for loops.
@jtorrents
Copy link
Member Author

@ysitu, I've added the node attribute members with the list of nodes in the SCC. And also improved the condensation function as you suggested. Maybe having both mapping as a graph attribute and members as node attribute is a bit redundant. I don't think is a big problem, but if we have to keep only one, I'd prefer to keep the node attribute because, as you said, it's a nice way to expose directly the computed SCCs.

ysitu added a commit that referenced this pull request May 11, 2014
Add mapping dict as graph attribute in condensation.
@ysitu ysitu merged commit 0de61b7 into networkx:master May 11, 2014
@jtorrents jtorrents added this to the networkx-1.9 milestone May 11, 2014
@jtorrents jtorrents deleted the condensation-mapping branch May 12, 2014 15:03
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