Skip to content
This repository was archived by the owner on May 1, 2024. It is now read-only.

Experimental MultiGraph support#63

Open
Lamaun wants to merge 1 commit intonetworkx:mainfrom
Lamaun:master
Open

Experimental MultiGraph support#63
Lamaun wants to merge 1 commit intonetworkx:mainfrom
Lamaun:master

Conversation

@Lamaun
Copy link
Contributor

@Lamaun Lamaun commented Nov 3, 2019

See #62

@Lamaun
Copy link
Contributor Author

Lamaun commented Nov 16, 2019

Should I write some tests for this? Or is something else keeping this from a merge?

Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

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

Sorry this is not getting much attention.
Here's a review:

  • I'm not sure this change will work with the METIS adjacency_list data structure. But I assume you have checked that it works. Can you add some tests -- they will both check that something doesn't mess it up later, and serve as a very basic form of documentation of how it should work.

  • dict(G[u])[v] should be simplified to G[u][v] ... or you could make the inner loop:

for v, keydict in G[u].items():
    adjncy.extend(index[v] for key in keydict)

- docstrings for private functions don't matter much, but could you change _convert_multi_graph and _convert_graph to docstrings like:

    """Convert a Graph to the numbered adjacency list expected by METIS.
    """

That way they are a single line and under 80 chars so Sphinx and pycodestyle are happy.

- To allow subclasses of networkx graphs, replace ```if(isinstance(G,nx.MultiGraph)):``` with ```if G.is_multigraph()):``` 

Thanks!

Base automatically changed from master to main March 2, 2021 17:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants