ENH: always store x, y as node attributes in gdf_to_nx, optionally cast node labels to integers

[martinfleis]

This supersedes #545 (sorry @anastassiavybornova!). While getting into a review of #545, I wrote this to verify my hypothesis and given it is way simpler and way more generic, it is preferable solution.

We'll also need to adapt nx_to_gdf to support the option with integer labels but will do that as a follow-up.

• tests

closes #545

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2ceac22) 97.4% compared to head (96488b7) 97.4%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #546   +/-   ##
=====================================
  Coverage   97.4%   97.4%           
=====================================
  Files         26      26           
  Lines       4317    4328   +11     
=====================================
+ Hits        4203    4214   +11     
  Misses       114     114           

Files	Coverage Δ
momepy/tests/test_graph.py	100.0% <ø> (ø)
momepy/tests/test_utils.py	99.2% <100.0%> (+0.1%)	⬆️
momepy/utils.py	98.8% <100.0%> (+<0.1%)	⬆️

[martinfleis]

We'll also need to adapt nx_to_gdf to support the option with integer labels but will do that as a follow-up.

We won't

momepy/momepy/utils.py

Lines 282 to 283 in 1224778

	if isinstance(node_xy[0], int) and "x" in node_data[0]:
	geometry = [Point(data["x"], data["y"]) for data in node_data] # osmnx graph

[jGaboardi]

Looks good to me. Shall we get @anastassiavybornova's eyes on it?

[martinfleis]

Shall we get @anastassiavybornova's eyes on it?

Ideally

[anastassiavybornova]

hi! cool! let me have a closer look at this tomorrow - the way i saw it the main issue with the previous version of _generate_primal was how the key (as in u,v,key) attribute was generated. (which is relevant for e.g. the osmnx.simplification function to not just run, but to return properly simplified linestrings)

[martinfleis]

the main issue with the previous version of _generate_primal was how the key (as in u,v,key) attribute was generated

could that be a bug more than anything else?

[martinfleis]

the main issue with the previous version of _generate_primal was how the key (as in u,v,key) attribute was generated

could that be a bug more than anything else?

Not strictly a bug but laziness. It seems that in the meantime, networkx learned to deal with it automatically, so I've removed any custom handling of keys. Now it should match 0...n behaviour you implemented in your PR.

[jGaboardi]

Since this is passing, do we think it a good idea to go ahead and merge? Then we can get @anastassiavybornova's retroactive review when she has time?

[martinfleis]

Let's wait, there's no rush, is there?

[jGaboardi]

Let's wait, there's no rush, is there?

I suppose not.

[anastassiavybornova]

hi again! so here's my 2 cents:

• indeed networkx is taking care of keys automatically now (like they say here: If some edges connect nodes not yet in the graph, the nodes are added automatically. If an edge already exists, an additional edge is created and stored using a key to identify the edge.)
• since that is the case, agree @martinfleis with your more generic & preferred solution
• only caveat left from my side; it all rests on the assumption that the linestrings in the input geodataframe are already "clean enough" as in "we are sure that only the start and endpoints of each linestring are (potentially) relevant intersections/nodes in the network". but this can be taken care of by preprocessing of course, doesn't need to happen within the _generate_primal function - just good to have in mind. in other words, ideally, the input geodataframe to gdf_to_nx(osmnx_like=True) should consist of only 2-point linestrings (as in: LineString([p1,p2])). then all the further osmnx simplification functions (like this updated one, simplify(attributes=...) ) should work fine. and there are may be already some momepy or geopandas functions that would help with this preprocessing?

[martinfleis]

we are sure that only the start and endpoints of each linestring are (potentially) relevant intersections/nodes in the network

That was always the assumption here. Or more specifically, this function does not care and ensures no change of information between the two representations of the same.

ideally, the input geodataframe to gdf_to_nx(osmnx_like=True) should consist of only 2-point linestrings (as in: LineString([p1,p2]))

How come? 2-point linestring either means radical simplification of input geometries or their division to individual segments, causing a ton of degree-2 nodes. Let's pick it up tomorrow.

and there are may be already some momepy or geopandas functions that would help with this preprocessing?

We do have some tooling, like removal of degree 2 nodes but I'd need to better understand the expected outcome.

[anastassiavybornova]

yes to the ton of degree 2 nodes --> that's what osmnx.simplification.simplify_graph expects as input. but yeah, let's discuss tomorrow! let's see if we deem it necessary to cater for this

[martinfleis]

Okay. But that does not change the scope of gdf_to_nx, so merging.