ENH: improved node consolidation functions

[gsagostini]

Function for consolidating intersections on street networks based on graph-distances along with edge reconstruction. Tests to come.

[codecov]

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (24672bf) 97.5% compared to head (e0949e4) 97.4%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #377     +/-   ##
=======================================
- Coverage   97.5%   97.4%   -0.2%     
=======================================
  Files         26      26             
  Lines       4189    4317    +128     
=======================================
+ Hits        4086    4203    +117     
- Misses       103     114     +11     

Files	Coverage Δ
momepy/datasets/__init__.py	100.0% <100.0%> (ø)
momepy/tests/test_preprocess.py	100.0% <100.0%> (ø)
momepy/preprocessing.py	93.3% <90.2%> (-0.9%)	⬇️

[martinfleis]

have you tried using difference instead? It may be more efficient that split + linemerge (but may not and may cause more trouble than good).

[martinfleis]

@gsagostini have you had a chance to work on tests?

[martinfleis]

@gsagostini Have you closed it intentionally?

[gsagostini]

I discarded edits while trying to fetch upstream in order to bring my fork up to date. Will open the PR again soon with the new commits.

[gsagostini]

@martinfleis @jGaboardi I reopened it with the tests and updated commits.

[jGaboardi]

@gsagostini Did you run black with pre-commit for code in this PR before pushing up the latest commits?

[gsagostini]

@jGaboardi I had ran black only in the momepy directory but now it should have been fine for the tests too

[martinfleis]

You will need to add consolidate_intersections to the list called __all__ on top of the file.

[gsagostini]

Tests are still failing for the consolidate_intersections function on the minimal environment. Is this expected? Seems like the tests fail for other functions too, not just for the one I added

[martinfleis]

Thanks! This is a great PR! Left just a couple of minor comments.

[gsagostini]

@jGaboardi @martinfleis thanks! yes those are all commits I wanted to do in the preprocessing.py file. I need to fetch the new user guide structure (with the preprocessing directory) to add the notebook, is it better to do it in a new PR once this one gets merged? Also I think CodeFactor check is not successful, but I can't see what lines of code are causing it---is it an issue?

[martinfleis]

@gsagostini I synced this with main. You will need to pull the changes.

flake8 is complaining about long lines in docstrings, you'll need to fix that. CodeFactor can be ignored.

[gsagostini]

@martinfleis sounds good will do. Is flake8 something I can install and run like black?

[martinfleis]

Yes -> https://flake8.pycqa.org/en/latest/. We also use isort now to check the order of imports. The easiest way of ensuring it is all checked on every commit is to install a pre-commit hook.

You propably must first install pre-commit:

pip install pre-commit

From the root of the repository, you should then install the pre-commit included in momepy:

pre-commit install

Then black and flake8 and sort will be run automatically each time you commit changes. You can skip these checks with git commit --no-verify.

[gsagostini]

Great! I added the notebook too. Let me know if there is more to be said on the user guide, and I will take a look :)

EDIT: Seems like there is a notebook error from building the docs. Let me know if it's easier to create a separate PR for the user guide.

[martinfleis]

@gsagostini It would be great if this can be finished soon, so we can release 0.6. Any updates from your side? Do you have a bit of time or shall we try to resolve remaining issues?

[martinfleis]

I have revived this. Resolved conflicts, fixed shapely 2.0 compatibility, fixed some minor lint issues.

I did not do thorough review of the code hoping that my former self in 2022 did :D. I believe this can be merged now. All this functionality will undergo some testing and momepy 1.0 may or may not include it, depending on the results.

[jGaboardi]

hoping that my former self in 2022 did :D.

Dangerous assumption 🤣

[jGaboardi]

All suggestions are minor doc adjustments.

[jGaboardi]

@martinfleis shall we squash & merge?

[martinfleis]

@gsagostini Thanks for all the work that went into this!

[jGaboardi]

As Martin said, congrats @gsagostini !!! 🎉