Skip to content

Conversation

@csebastiao
Copy link

See #591 for explanation. Drafted without full tests and documentation. We ran the code with https://github.com/pysal/momepy/blob/main/ci/envs/312-latest.yaml.

Contribution

Adding a strokegraph.py file with 2 main functions:

  • strokes_to_graph taking momepy.COINS as an input and returning a dual graph where each node is a stroke in momepy.COINSand each edge is when two strokes are intersecting. Optional arguments include computing additional metrics on the strokes (degree, closeness, betweenness, connectivity, access, orthogonality, and spacing, see (El Gouj et al., 2022) for details), and returning the primal graph of the street network with the information about each stroke on every edge composing them.
  • graph_to_strokes taking the dual graph generated by strokes_to_graph as an input and returning a momepy.COINS object as an output.

Questions

  • Do we need graph_to_strokes, as it returns the output from momepy.COINS ?
  • Does the structure of strokes_to_graph where the return of the primal graph is optional make sense ?

TODOs

  • Finish the documentation.
  • Finish test coverage.

@codecov
Copy link

codecov bot commented Jul 24, 2025

Codecov Report

❌ Patch coverage is 49.07407% with 55 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.0%. Comparing base (4037c70) to head (6d170ee).
⚠️ Report is 129 commits behind head on main.

Files with missing lines Patch % Lines
momepy/strokegraph.py 32.1% 55 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #704     +/-   ##
=======================================
- Coverage   97.4%   97.0%   -0.3%     
=======================================
  Files         26      28      +2     
  Lines       4328    4303     -25     
=======================================
- Hits        4214    4176     -38     
- Misses       114     127     +13     
Files with missing lines Coverage Δ
momepy/__init__.py 100.0% <100.0%> (ø)
momepy/tests/test_strokegraph.py 100.0% <100.0%> (ø)
momepy/strokegraph.py 32.1% <32.1%> (ø)

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@anastassiavybornova
Copy link
Contributor

anastassiavybornova commented Oct 30, 2025

  • graph_to_strokes: pointless -- can be simply dropped @anastassiavybornova
  • for metrics that are vanilla networkx: do NOT compute separately. @anastassiavybornova
  • we do not need to return primal graph at all (anvy: make sure we can recreate -- mapping through geopandas) @anastassiavybornova
  • have a function that only transforms data structure coins>graph. instead of "strokes_to_graph", call it "coins_to_nx". @anastassiavybornova
  • for metrics that are derived: separate functions, if they rely on some vanilla stuff >> have it there inside the derived-metric function. in momepy, the logic is: every metric has its own function. steps: create graph > pass it to function > return metric. @anastassiavybornova
  • update documentation of each metric (in the separate functions, explain how metric is computed and what literature it is based on) @csebastiao
  • in examples/strokegraph.ipynb, showcase: visualize coins-to-nx process and metrics for Bubenec @csebastiao

once api is fixed, ping martin, get thumbs up, then we can work on tests.

@csebastiao
Copy link
Author

Functions should be fixed, with added documentation and an example in a jupyter notebook. Tell me what you think @martinfleis (and @anastassiavybornova for the documentation) and then we can continue with the tests !

Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

Cool! This looks much better and super close to what I think is mergeable. Some comments left in the code.

Comment on lines +80 to +84
>>> gdf = momepy.remove_false_nodes(gdf)
>>> primal_graph = momepy.gdf_to_nx(gdf, preserve_index=True, approach="primal")
>>> lines = momepy.nx_to_gdf(primal_graph, points=False, lines=True)
>>> coins = momepy.COINS(lines)
>>> stroke_graph = momepy.coins_to_nx(coins)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>> gdf = momepy.remove_false_nodes(gdf)
>>> primal_graph = momepy.gdf_to_nx(gdf, preserve_index=True, approach="primal")
>>> lines = momepy.nx_to_gdf(primal_graph, points=False, lines=True)
>>> coins = momepy.COINS(lines)
>>> stroke_graph = momepy.coins_to_nx(coins)
>>> coins = momepy.COINS(gdf)
>>> stroke_graph = momepy.coins_to_nx(coins)

This should work as well, no?

Comment on lines +62 to +64
Creates the stroke graph of a street network. The stroke graph is similar to, but
not identical with, the dual graph. In the stroke graph, each stroke (see
``momepy.COINS``) is a node; and each intersection between two strokes is an edge.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Creates the stroke graph of a street network. The stroke graph is similar to, but
not identical with, the dual graph. In the stroke graph, each stroke (see
``momepy.COINS``) is a node; and each intersection between two strokes is an edge.
Creates the continuity stroke graph of a street network. The continuity stroke graph is similar to, but
not identical with, the dual graph. In the continuity stroke graph, each stroke (see
``momepy.COINS``) is a node; and each intersection between two strokes is an edge.

Parameters
----------
coins: momepy.COINS
Strokes computed from a street network.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Strokes computed from a street network.
Continuity strokes computed from a street network.

Comment on lines +94 to +96
stroke_gdf["rep_point"] = stroke_gdf.geometry.apply(
lambda x: x.interpolate(0.5, normalized=True)
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stroke_gdf["rep_point"] = stroke_gdf.geometry.apply(
lambda x: x.interpolate(0.5, normalized=True)
)
stroke_gdf["rep_point"] = stroke_gdf.geometry.interpolate(0.5, normalized=True)

Vectorized ops are always preferred. There's nearly no need for apply in geopandas.

def stroke_connectivity(stroke_graph):
"""
Computes the stroke's connectivity. Connectivity is defined as
the number of arcs a stroke intersects. Comparing to the degree,
Copy link
Member

Choose a reason for hiding this comment

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

What is an "arc" in this context?

Comment on lines +181 to +184
Computing dual graph
---------------------

Set of functions to create a dual graph and compute metrics on:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Computing dual graph
---------------------
Set of functions to create a dual graph and compute metrics on:
Computing continuity graph
---------------------------
Set of functions to create a continuity graph and compute metrics on:

Copy link
Member

Choose a reason for hiding this comment

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

"It is a dual graph as defined in Porta, S., Crucitti, P. and Latora, V., 2006." - is it? That is not based on continuity strokes.

Copy link
Member

Choose a reason for hiding this comment

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

from matplotlib import pyplot as plt -> import matplotlib.pyplot as plt

Copy link
Member

Choose a reason for hiding this comment

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

Why do you go through the primal graph there? Seems wasteful.

Copy link
Member

Choose a reason for hiding this comment

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

I could go on but I might take a stab at revising this notebook myself later on. Generally it feels a bit too complicated. A user guide should narrowly focus whatever is being explained without other complications. there's a lot of additional code in here that is more a distraction than anything else. Like the conversion to nx graph, all those multilayer plots, stuff like stroke_betweenness_dict mapping and so on. Keep the notebook as is for now though. Might be easier for me to do that than trying to explain myself.

"""
point = tuple(point)
coords = list(linestring.coords)
assert point in coords, "point not on linestring!"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert point in coords, "point not on linestring!"

You got this covered by the more appropriate ValueError below.

Comment on lines +103 to +105
stroke_gdf["edge_indeces"] = stroke_gdf.stroke_id.apply(
lambda x: list(stroke_attribute[stroke_attribute == x].index)
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stroke_gdf["edge_indeces"] = stroke_gdf.stroke_id.apply(
lambda x: list(stroke_attribute[stroke_attribute == x].index)
)
stroke_attribute.groupby(stroke_attribute).apply(lambda group: group.index.tolist())

No for-loops over rows :). This is much faster.

Comment on lines +110 to +118
# Add stroke ID to each edge on (primal) graph
nx.set_edge_attributes(
G=graph,
values={
e: int(stroke_attribute[graph.edges[e]["index_position"]])
for e in graph.edges
},
name="stroke_id",
)
Copy link
Member

Choose a reason for hiding this comment

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

I don't get the point of this. graph already has stroke_id on edges coming from gdf_to_nx. It is a wrong one or what?

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.

3 participants