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

Return networkx graph #41

Merged
merged 15 commits into from
Mar 23, 2022
Merged

Return networkx graph #41

merged 15 commits into from
Mar 23, 2022

Conversation

amanmajid
Copy link
Contributor

Potential solution to #39

Added blank line at the end
Potential solution for tomalrussell#40, but relies on my solution to tomalrussell#39
src/snkit/network.py Outdated Show resolved Hide resolved
src/snkit/network.py Outdated Show resolved Hide resolved
src/snkit/network.py Outdated Show resolved Hide resolved
src/snkit/network.py Outdated Show resolved Hide resolved
@tomalrussell
Copy link
Owner

tomalrussell commented Sep 6, 2021

Thanks for working on this @amanmajid 😊 .. will be super useful

Apart from the notes above, could you:

  • add tests for each new method
  • add networkx to setup.py extras and dev-rquirements
  • add a bit more to the docs - what do you think would be most helpful? Maybe a section in the README and/or demo notebook?

What do you reckon about @czor847's suggestion of a to_igraph method - could you add it too while you're looking at this related stuff?

@amanmajid
Copy link
Contributor Author

amanmajid commented Sep 8, 2021

No worries!

I've just made some changes to the code as requested. In terms of the additional bullets:

  • I will add tests once you've checked and approved the revisions.
  • Will do. However, should we do this since you said networkx is should be an optional dependency?
  • Hmm... I think we should update the README once we have several functions interacting with other networks libraries. So for now I will just add a cell to the existing notebook.
  • I could add a similar function for the to_igraph - @czor847 do you have any existing code that I can adapt?

@tomalrussell
Copy link
Owner

Great 😊

I wonder if we should create the networkx graph as a MultiDiGraph - that way it can handle multiple edges between pairs of nodes, and will retain the direction of edges, both of which are okay within the GeoDataFrame representation. It should also keep snkit in line with osmnx and momepy in terms of exchanging networkx graphs.

In terms of recording requirements - yes, I’d put it on a line in “extras” so that pip will only install networkx if you do something like ‘pip install snkit[nx]’ and in dev-requirements so we can test the optional functionality.

Copy link
Owner

@tomalrussell tomalrussell left a comment

Choose a reason for hiding this comment

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

Some detail-oriented comments! But also a suggested change to how we construct the networkx graph that would make a reasonably large difference to the API.

src/snkit/network.py Outdated Show resolved Hide resolved
src/snkit/network.py Outdated Show resolved Hide resolved
src/snkit/network.py Outdated Show resolved Hide resolved
# get edges from network data
edges_as_list = list(zip(network.edges.from_id,network.edges.to_id))
# add edges to graph
G.add_weighted_edges_from(edges_as_list)
Copy link
Owner

Choose a reason for hiding this comment

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

Should we pass in a column to use as weight here?

Copy link
Owner

Choose a reason for hiding this comment

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

Just thinking about this - it would be generally useful to add all the edge attributes here, including geometry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed this part to extract nodal positions and edge weights (https://github.com/amanmajid/snkit/blob/e55bc44c4aa848fb9f1724f8db504a3c6a7ad114/src/snkit/network.py#L613-L639). I'm not sure how to go about efficiently adding all edge attributes when defining the graph?

src/snkit/network.py Outdated Show resolved Hide resolved
src/snkit/network.py Outdated Show resolved Hide resolved
# init graph
G = nx.Graph()
# get nodes from network data
G.add_nodes_from(network.nodes.id.to_list())
Copy link
Owner

Choose a reason for hiding this comment

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

Similarly to edges, can we add node attributes to the graph here?

src/snkit/network.py Outdated Show resolved Hide resolved
@tomalrussell
Copy link
Owner

Thanks for this, @amanmajid - closes #39 and #40, potential follow-up in #42

@tomalrussell tomalrussell merged commit 5fe7b19 into tomalrussell:master Mar 23, 2022
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.

2 participants