dedicated minimal tests -- nodes.remove_false_nodes()

[jGaboardi]

This PR:

• adds dedicated minimal tests for nodes.remove_false_nodes()
• resolves rough edge in sgeop.nodes.remove_false_nodes when multilinestrings passed #60
• xref
  • start testing + refactor [stub issue] #21
  • testing, refactor, & docstrings #20
  • ENH: do not fail with 3d nodes - preprocess.remove_false_nodes() pysal/momepy#661
• adds testing
• adds docstrings and type hints

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.3%. Comparing base (35bd6a0) to head (b9653b5).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main     #59     +/-   ##
=======================================
+ Coverage   98.1%   98.3%   +0.2%     
=======================================
  Files          6       6             
  Lines        894     894             
=======================================
+ Hits         877     879      +2     
+ Misses        17      15      -2     

Files with missing lines	Coverage Δ
sgeop/nodes.py	98.8% <100.0%> (+1.2%)	⬆️

... and 1 file with indirect coverage changes

[martinfleis]

We should probably also copy all the tests from momepy here.

[jGaboardi]

We should probably also copy all the tests from momepy here.

We are not expecting the same results here, though right? Just trying the first little block locally and it fails.

        false_network = geopandas.read_file(momepy.datasets.get_path("tests"), layer="network")
        false_network["vals"] = range(len(false_network))
        fixed = sgeop.nodes.remove_false_nodes(false_network)
>       assert len(fixed) == 56
E       assert 61 == 56
E        +  where 61 = len(                                                geometry  vals\n61.0   LINESTRING (-727482.257 -1052966.07, -727485.5...     0\n62.0   LINESTRING (-727210.943 -1053072.14, -727212.8...     1\n63.0   LINESTRING (-727210.943 -1053072.14, -727206.5...     2\n64.0   LINESTRING (-727141.085 -1053103.182, -727129....     3\n65.0   LINESTRING (-727136.904 -1053091.833, -727117....     4\n...                                                  ...   ...\n117.0  LINESTRING (-727412.897 -1052948.823, -727407....    56\n118.0  LINESTRING (-727248.835 -1052881.77, -727254.5...    57\n119.0  LINESTRING (-727289.896 -1052909.828, -727281....    58\n120.0  LINESTRING (-727227.04 -1052735.582, -727226.1...    59\n121.0  LINESTRING (-727238.493 -1052817.281, -727253....    60\n\n[61 rows x 2 columns])

sgeop/tests/test_nodes.py:399: AssertionError

[martinfleis]

Not entirely sure on those test data to be fair. The one in sgeop does contain some fixes but it may be worth verifying the differences.

[jGaboardi]

Not entirely sure on those test data to be fair. The one in sgeop does contain some fixes but it may be worth verifying the differences.

OK after the PR on the momepy side is settled I will back in this.

[jGaboardi]

This seems to specifically be happening in the sgeop implementation when passing in a GeoSeries -- passing in a GeoDataFrame succeeds.

Wrong, I was using momepy.remove_false_nodes() instead of sgeop.remove_false_nodes(). This is still very much unsolved from both frames and series.

[jGaboardi]

Key piece in momepy.remove_false_nodes() that is not in sgeop.remove_false_nodes():

# explode to avoid MultiLineStrings
 # reset index due to the bug in GeoPandas explode
 df = gdf.reset_index(drop=True).explode(ignore_index=True)

[jGaboardi]

When adding the line above into sgeop.remove_false_nodes() all chunks here pass (when some index tinkering) except for the coordinate ordering chunk:

# check loop order
expected = np.array(
    [
        [-727238.49292668, -1052817.28071986],
        [-727253.1752498, -1052827.47329062],
        [-727223.93217677, -1052829.47624082],
        [-727238.49292668, -1052817.28071986],
    ]
)
np.testing.assert_almost_equal(
    np.array(fixed.loc[53].geometry.coords), expected
)

This appears to be due to the indices of the resultant edges not lining up, and the index location produced from the sgeop implementation is now 55. Can confirm testing suite passes locally with this adjustment.

[martinfleis]

Let's also add notes on 3D geometry being downcast, potentially. And about MultiLineStrings being exploded.