-
Notifications
You must be signed in to change notification settings - Fork 61
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
gdf2osmnx #545
gdf2osmnx #545
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #545 +/- ##
=======================================
- Coverage 97.4% 97.3% -0.1%
=======================================
Files 26 26
Lines 4317 4358 +41
=======================================
+ Hits 4203 4240 +37
- Misses 114 118 +4
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking very good @anastassiavybornova.
- break each linestring in gdf into two-point linestrings
- for each two-point linestring, find corresponding node IDs (u,v)
I am curious, does this approach handle loops (e.g.,
blg.loc[ | ||
blg.loc[blg["mm_uid"] == key].index[0], blg.geometry.name | ||
] = new_geom | ||
blg.loc[blg.loc[blg["mm_uid"] == key].index[0], blg.geometry.name] = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this extra indexing needed due to an update in Pandas/GeoPandas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haven't touched that one (except for running black on it) i'll leave the question to @martinfleis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did only a partial review as I need to clarify some bits.
What exactly is the difference between the two options? Assuming both are MultiDiGraphs. As far as I can tell, the only difference is that the original uses tuple (x, y)
to encode node with no attributes, the osmnx-like encodes nodes to integers and adds x and y as attributes. Is there anything else I don't see?
If I am right, then my suggestion would be to adapt our original function to add {'x': 1, 'y': 2}
attributes to each node as osmnx-like version do and if osmnx_like=True, call convert_node_labels_to_integers
and be done with it. Am I wrong?
else: | ||
graph.add_edge(first, last, **attributes) | ||
gdf_network["geometry"] = gdf_network["geometry"].apply( | ||
lambda x: linemerge(x) if x.geom_type == "MultiLineString" else x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you doing this? If it is a pre-processing step, than a user should do it themselves. Otherwise this does not round-trip as it should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_generate_primal already checks for some potential non-linestring geometries, so i thought i'd add this one as well - but yes it is a pre-processing step indeed. fine to leave it out of the function.
Looking into it a bit further, it seems that the only change we need to do is to add the attributes with coordinates to nodes. Adding these two lines results in no issue on a small sample dataset. node_attrs = {node: {"x": node[0], "y": node[1]} for node in graph.nodes}
nx.set_node_attributes(graph, node_attrs) |
Co-authored-by: James Gaboardi <[email protected]>
Co-authored-by: James Gaboardi <[email protected]>
@anastassiavybornova maybe have a look at #546 before spending more time on this. |
Added the
osmnx_like
keyword togdf_to_nx()
. Ifosmnx_like=True
, within_generate_primal
:net
with unique dummy IDsgdf_to_nx()
now works as input forosmnx.simplify_graph()