pysal / momepy

Urban Morphology Measuring Toolkit
https://docs.momepy.org
BSD 3-Clause "New" or "Revised" License
496 stars 59 forks source link

gdf_to_nx only uses first and last points of geometry (LineStrings) #499

Closed nlaws-camus closed 1 year ago

nlaws-camus commented 1 year ago

LineStrings can contain more than two points but the _generate_primal method used in gdf_to_nx only parses the first and last points:

https://github.com/pysal/momepy/blob/2fda11ef6ec14deb2dc9a53ffcf8c1576859ebea/momepy/utils.py#L75-L77

Instead, _generate_primal should loop through row.geometry.coords[0:-1] and graph.add_edge for each value, e.g.

key = 0
for row in gdf_network.itertuples():
    data = list(row)[1:]
    attributes = dict(zip(fields, data))
    for i, first in enumerate(row.geometry.coords[0:-1]):
        last = row.geometry.coords[i+1]
        if multigraph:
            graph.add_edge(first, last, key=key, **attributes)
            key += 1

            if oneway_column:
                oneway = bool(getattr(row, oneway_column))
                if not oneway:
                    graph.add_edge(last, first, key=key, **attributes)
                    key += 1
          else:
              graph.add_edge(first, last, **attributes)
martinfleis commented 1 year ago

No, it should not.

What we're doing here is transforming a representation of a network where each LineString encodes a portion of, for example, road network between two logical nodes, i.e. junctions. This portion may be straight or super curvy but it is still one portion (I am intentionally avoiding the word segment), which shall be represented in a graph as a single edge between two nodes. The geometry itself is stored as an edge attribute so it is not lost.

Your proposal creates a different graph. One that does not encode logical portions of a network into a graph but one that encodes the way someone has drawn the geometry. You can have a curve composed of 10 segments of LineString or 100 or even 1000 if you want to have a really smooth shape. Each of those encodes the same part of a road network, so each should be a single edge between two nodes. While in your case, you would end up with vastly different graphs encoding different relationships between physical elements. Moreover, you would have a ton of nodes of a degree 2 which are often considered superfluous (but that depends on the type of a graph of course).

I can imagine that a graph you seek may have some use cases but those are unlikely within the scope of momepy.

Happy to hear the use case and why you think what we do is wrong in there, though.