momepy icon indicating copy to clipboard operation
momepy copied to clipboard

ENH: helper functions for geometry-based network simplification

Open gregmaya opened this issue 3 years ago • 9 comments

I wanted to leave this PR open to be able to refer to it in my GSOC README. I know there are quite a few things still to be done here so might be worth holding back a very thorough review

gregmaya avatar Sep 12 '22 13:09 gregmaya

Codecov Report

Merging #396 (6789d1b) into main (afcb85b) will decrease coverage by 2.2%. The diff coverage is 10.4%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #396     +/-   ##
=======================================
- Coverage   95.9%   93.7%   -2.2%     
=======================================
  Files         13      13             
  Lines       2965    3041     +76     
=======================================
+ Hits        2842    2849      +7     
- Misses       123     192     +69     
Impacted Files Coverage Δ
momepy/preprocessing.py 78.9% <10.4%> (-12.4%) :arrow_down:

codecov[bot] avatar Sep 12 '22 13:09 codecov[bot]

@gregmaya what is the state of this and a tentative timeline?

martinfleis avatar Oct 10 '22 18:10 martinfleis

Hey Martin. Thanks for reaching out. I was thinking I would better come back to this before the ideas escape my mind. I've been rather busy trying to find payed projects 😔... But let me investigate this tomorrow to give you a better estimate.

gregmaya avatar Oct 10 '22 19:10 gregmaya

One more ping @gregmaya :).

martinfleis avatar Jan 01 '23 21:01 martinfleis

@martinfleis I feel very sorry that I've not been able to contribute more to this. As a matter of fact, my coding skills have probably suffered some rustiness as I've not been doing much coding in the last couple of months. However, as part of my 2023 resolutions, I wanted to resurface this as a priority. So I have given one day a week (most likely Tuesdays) to continue with these functions...

If I'm not wrong my last blocking was trying to generate the number of forming edges to each of the polygons. Ideally, this could also be used in the selection of polygons that are adjacent to roundabouts (as discussed last year). I don't seem to remember exactly the cases where I was getting an error but I'll try to replicate it and come back to you.

I should be back online from the 11th Jan. HNY for you all !!

gregmaya avatar Jan 03 '23 10:01 gregmaya

Hi @martinfleis and @jGaboardi. I do feel very sorry that I have not been able to keep up with this as we all would've liked. Lately, I've tried to spend some time looking into this and I've been thinking about realistic ways to move forward. In short, my aim is to have small contributions rather than accumulate too many functions to then come back to a PR.

I have to admit that from the round_about_simplification() only one thing I ended up not very happy with: the way we were selecting the 'adjacent polygons' ... the ideal would be to select those on the basis of forming edges (not on their area-like it currently is). I have now developed that function. I know this might feel like going backwards but believe this is pivotal to moving forward because adding that attribute to the polygons_gdf would allow us to select the single_tri_like_junctions (and potentially many more).

see this for reference.

but let me know which way you think is best to proceed. i.e. maybe a new branch? the current one is very much out of date and is giving me many import warnings on most of the main libraries 🧐

gregmaya avatar Jan 30 '23 16:01 gregmaya

Okay, that is fine.

maybe a new branch? the current one is very much out of date and is giving me many import warnings on most of the main libraries

Make a branch for every contribution that will turn into a PR. You can also update this one but given the scope will change, it may be better to close it and open new, narrower ones.

martinfleis avatar Jan 30 '23 20:01 martinfleis

OK brill. If possible let's leave this open. It helps me for referencing some stuff. Once the others achieve what this one was supposed to do I'll close it.

gregmaya avatar Jan 31 '23 11:01 gregmaya

If possible let's leave this open. It helps me for referencing some stuff. Once the others achieve what this one was supposed to do I'll close it.

Closing does not mean it goes anywhere, you will still see all you can now. It will just be clearly marked as not expected to be merged.

martinfleis avatar Jan 31 '23 12:01 martinfleis

This is not happening anytime soon. Closing to clear the backlog.

@gregmaya note that there's an ongoing effort to develop a new simplification algo so it may be eventually all superseded by that.

martinfleis avatar Jun 15 '24 13:06 martinfleis

Makes sense to close for backlog clarity

Would love to see what the thinking around simplification is now.

Also, if there's room for me to collaborate, would love to participate!

gregmaya avatar Jun 15 '24 14:06 gregmaya