Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: helper functions for geometry-based network simplification #396

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gregmaya
Copy link
Contributor

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 gregmaya changed the title Initial commit of helper functions for single_tri_junctions ENH: helper functions for geometry-based network simplification Sep 12, 2022
@jGaboardi jGaboardi added enhancement New feature or request GSoC 2022 labels Sep 12, 2022
@codecov
Copy link

codecov bot commented Sep 12, 2022

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%) ⬇️

@martinfleis
Copy link
Member

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

@gregmaya
Copy link
Contributor Author

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.

@martinfleis
Copy link
Member

One more ping @gregmaya :).

@martinfleis martinfleis added this to the 0.6.0 milestone Jan 1, 2023
@gregmaya
Copy link
Contributor Author

gregmaya commented Jan 3, 2023

@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 !!

@martinfleis martinfleis mentioned this pull request Jan 12, 2023
@gregmaya
Copy link
Contributor Author

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 🧐

@martinfleis
Copy link
Member

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.

@gregmaya
Copy link
Contributor Author

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.

@martinfleis
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants