ENH: helper functions part 1

[gregmaya]

This is the correction that I really wanted to make before moving back to fixing the tri_like_junctions. Apologies for any code clumsiness. I'm afraid some of my python skills might need refreshing.

[codecov[bot]]

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@50f29d3). Click here to learn what that means. The diff coverage is 100.0%.

:exclamation: Current head 62eab1f differs from pull request most recent head f1d67a2. Consider uploading reports for the commit f1d67a2 to get more accurate results

@@          Coverage Diff           @@
##             main    #465   +/-   ##
======================================
  Coverage        ?   96.1%           
======================================
  Files           ?      13           
  Lines           ?    2974           
  Branches        ?       0           
======================================
  Hits            ?    2857           
  Misses          ?     117           
  Partials        ?       0           

Impacted Files	Coverage Δ
momepy/preprocessing.py	91.9% <100.0%> (ø)

[gregmaya]

Happy to discuss the new processes! But actually more excited to be able to use the new helper function to identify tri_like_junctions... :P

[martinfleis]

Could you summarise what this does? Need to know the goal to properly review it :).

[gregmaya]

In short, all I did was include a few attributes to the polygons before selecting them as part of the overall roundabout group or not. i.e. _create_shape_index() and _count_edges_per_polygon(). Then I use those two functions within the overall process that selects the adjacent polygons. For a polygon in order to be considered adjacent it now has to :

• be touching a circle (roundabout)
• be smaller than its paired circle OR
• have 3 forming edges AND
• be less than 4x the size of the roundabout (this number was after some testing but I still include it as part of the function's adjustable attributes)

This just creates a simpler (arguably more robust) way of dealing with those roads approaching roundabouts than the current test on the area and Hausdorff distance.

Now that I have had time to detach myself from this a little bit I do still see one problem with the overall roundabout_simplification() function, though. That is when two roundabouts share the same adjacent polygon the outputs are pretty bad, and their link gets broken. see image

Overall, this is an enhancement but the next step should be to fix those cases (luckily it's not very often than that occurs)

[gregmaya]

Hey @martinfleis @jGaboardi I believe I need your help here. I went ahead and updated the function to disregard the adjacent polygons that are shared amongst two (or more roundabouts). Up to that point all seems to be running OK. However, for a reason that I cannot understand I'm now getting a TypeError: 'NoneType' object is not subscriptable in a for loop (lines 979 - 983):

lines = []
for i, row in incoming.iterrows():
        if row.dist_first_pt < row.dist_last_pt:
            lines.append(LineString([row.first_pt, row.center_pt]))
        else:
            lines.append(LineString([row.last_pt, row.center_pt]))

This is trying to add a line from the center point of the roundabout to the closest end of the (previously selected) incoming lines.
I've tried to debug this several times, and even when I look at individual repetitions of the loop the lines do get added to the lines list. But when I run it all is that I get the error. Could it be something with the way shapely created coordinates?

Anyway, one thing fixed/improved and something else breaks !

[martinfleis]

hey sorry, I hope to get to check the code this week. if you can ensure CI is green in the meantime, that'd be fab!

[gregmaya]

hey sorry, I hope to get to check the code this week. if you can ensure CI is green in the meantime, that'd be fab!

Sure, I'm on it !

[jGaboardi]

@gregmaya Also, you can take care of the formatting issues/failure either installing pre-commit in your environment or running the following on preprocessing.py:

$ isort preprocessing.py
$ flake8 preprocessing.py
$ black preprocessing.py

Either option will require you to install isort, flake8, and black.

[jGaboardi]

xref #396

[gregmaya]

Hopefully, by keeping the PR as 'draft' I wasn't spamming you in every single commit 🙈

@jGaboardi thanks for the advice. Formatting is definitely giving me a hard time, lately.

[jGaboardi]

Hopefully, by keeping the PR as 'draft' I wasn't spamming you in every single commit

All good. I don't mind it at all.

[gregmaya]

Ok so before you go ahead and review this code I actually kept thinking that the hausdorff_distance bit (lines 860-869) was sub-optimal. I know it runs but it's not the most efficient and is hard to explain. I, therefore, went another route and I have an Enhancement to that which is faster (as it uses arrays) but before committing that I thought I would ask you guys if you prefer to have small changes at a time...

[martinfleis]

Generally yes but if waiting for a review should block you then go ahead and make a bigger change

[gregmaya]

Hey, @martinfleis and @jGaboardi just flagging that this is ready for review. Once approved, I'll use some of the functions here to speed up the resolution of 'tri-junctions' which was started in PR #396.

Also, are you guys thinking to include this as part of GSOC2023? I saw NumFOCUS there but couldn't find momepy in their list

[martinfleis]

Thanks! Will have a look.

Also, are you guys thinking to include this as part of GSOC2023

We haven't discussed this yet.

[gregmaya]

Thanks, Martin. I've seen your comments and hope to be back on this soon.

[gregmaya]

I tried to implement most of the suggestions. However, I must say that I couldn't figure out a way to avoid the for loop for calculating the max_distance to all vertex of the adjacent polygons. I did change it to using the distance() function but couldn't figure out how to avoid the loop. As for the failed checks, I believe those are mostly in relation to esda. I'm pretty sure it needs to be included in the different environments. Are you happy to include that?

[gregmaya]

@martinfleis @jGaboardi just raising some awareness of this as I believe the updated functions work as long as esda is incorporated into the test environments. Thanks

[martinfleis]

I've added esda to CI in 7548af0. Let's see how it goes, I haven't tested it locally.

[gregmaya]

Hey! Any idea why the pre-commit checks remain 'pending'? Is there anything I can do?

[martinfleis]

@gregmaya I managed to make it work :)

[martinfleis]

@gregmaya can you update the code to depend on shapely 2 instead of pygeos? We've just merged it elsewhere. There are also some to-do's when shapely 2 is available that can be tackled now.

[gregmaya]

Oh wow! Yeah I'll try to find some time this upcoming week. Thanks for the heads up!

[martinfleis]

Closing due to inactivity and needs of updating. The code will not go anywhere, so we can refer to it when we want to revisit this.