ENH: helper functions part 1 #465

gregmaya · 2023-02-01T10:55:03Z

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 · 2023-02-02T11:28:19Z

Codecov Report

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

❗ 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 · 2023-02-02T15:26:11Z

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 · 2023-02-02T19:12:32Z

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

gregmaya · 2023-02-02T20:11:31Z

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 · 2023-02-08T15:38:55Z

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 !

…ctions

martinfleis · 2023-02-13T11:53:41Z

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 · 2023-02-13T11:57:08Z

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 · 2023-02-13T15:32:47Z

@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 · 2023-02-13T15:33:47Z

xref #396

gregmaya · 2023-02-13T16:04:10Z

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 · 2023-02-13T16:13:51Z

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 · 2023-02-15T21:19:32Z

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 · 2023-02-15T21:21:32Z

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

gregmaya · 2023-02-23T10:43:11Z

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 · 2023-02-23T10:55:32Z

Thanks! Will have a look.

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

We haven't discussed this yet.

martinfleis

Thanks. A few notes in the code.

martinfleis · 2023-03-06T11:10:20Z

momepy/preprocessing.py

@@ -756,6 +757,57 @@ def _polygonize_ifnone(edges, polys):
    return polys


+def _create_shape_index(gdf):


There has been a development around this in the meantime leading to dropping of this specific formula for the shape index. I don't want to go into details now but the mathematical derivation of this one was flawed.

We now use this one in the paper (in progress)

from esda import shape polygons["circular_compactness"] = shape.minimum_bounding_circle_ratio(polygons) polygons["circular_compactness_index"] = polygons["circular_compactness"] * area

I suggest trying the same, but the threshold values will obviously change.

For sure, we should use esda to measure the actual index to avoid calling pygeos directly here (we'll need to get rid of pygeos soon).

I just reviewed this and the reality is that we only use reock as a selection metric for roundabouts. By using shape.minimum_bounding_circle_ratio(polygons) we would be simplifying the way we get those numbers (which is great) but the actual values are the same.
Either I'm missing something or we could just make those changes and forget about the extra step to create the INDEX that in reality never gets used.

martinfleis · 2023-03-06T11:11:30Z

momepy/preprocessing.py

+    polygons_gdf["geometry"] = polygons_gdf["geometry"].apply(
+        lambda x: make_valid(x) if not x.is_valid else x
+    )


This can be done as a vectorised call, make_valid is now available as GeoSeries.make_valid.

martinfleis · 2023-03-06T11:15:44Z

momepy/preprocessing.py

+        max_dists = []
+        for i, row in rab_adj.iterrows():
+            xs, ys = row.geometry.exterior.coords.xy
+            poly = np.array(list(map(lambda x, y: (x, y), xs, ys)))
+
+            cx, cy = row.savedgeom.centroid.coords.xy
+            c_point = np.array(list(map(lambda x, y: (x, y), cx, cy)))
+
+            # list of distances between centroid and each vertex of polygon
+            ds = map(np.linalg.norm, c_point - poly)
+            d_max = max(ds)
+            max_dists.append(d_max)


is this just a max distance between a centroid and a node? If so, you can avoid the loop and depend on shapely here for sure.

All I could think of was this...

max_dists = [] for _, row in rab_adj.iterrows(): dists = [ row.savedgeom_right.centroid.distance(Point(x, y)) for x, y in row.geometry.exterior.coords ] max_dists.append(max(dists)) rab_adj["max_dist"] = max_dists

but you probably had something else in mind.

gregmaya · 2023-03-15T18:09:50Z

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

gregmaya · 2023-03-16T14:03:38Z

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 · 2023-03-21T16:35:42Z

@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 · 2023-03-21T21:32:02Z

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

gregmaya · 2023-03-29T09:20:13Z

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

martinfleis · 2023-03-30T13:33:16Z

@gregmaya I managed to make it work :)

martinfleis

Can you make sure the test pass?

martinfleis · 2023-03-30T13:35:21Z

momepy/preprocessing.py

+    Counts the number of linestrings that formed the polygons
+    """
+    # Check if the polygons are valid geometries and fix them if not
+    polygons_gdf["geometry"] = polygons_gdf.geometry.make_valid()


This will fail for older geopandas (which we still formally support).

Also, we should call make_valid only if there are actually some invalid polygons.

martinfleis · 2023-03-30T13:36:11Z

momepy/preprocessing.py

    area_threshold_val = gdf.area.quantile(area_threshold)
-    rab = rab[rab[area_col] < area_threshold_val]
+    if area_threshold_val > 2000:


Any reason why 2000? Should this be controllable via a keyword?

martinfleis · 2023-04-08T15:56:51Z

@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 · 2023-04-08T15:59:14Z

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

gregmaya added 6 commits February 1, 2023 11:51

including two helper functions

4143349

formating string correction

2f666ea

black formatting on strings

015f558

second guessing where the formatting issue is

c6d713c

trailing whitespace fix (third time luck)

a0cb0c4

updating test checks

3b3e2d7

gregmaya changed the title ~~[ENH] helper functions part 1~~ ENH: helper functions part 1 Feb 2, 2023

gregmaya added 3 commits February 2, 2023 13:10

combined selection of adjacents: shape and area

1782891

selection function updated

0abc422

update to check on 'low_area_threshold'

a718939

gregmaya marked this pull request as ready for review February 2, 2023 15:10

gregmaya added 5 commits February 7, 2023 23:31

improving selection of adjacents when they touch more than one rab

7a0b1a8

black formating

e6aafde

black attempt 2

0e293fe

white spaces

e61f473

blank: line not containing whitespace?

538bdee

gregmaya marked this pull request as draft February 8, 2023 11:29

gregmaya added 2 commits February 13, 2023 12:51

replacing the area mask for a hausdorff_mask

5822e90

updating roundabout_simplification() to reflect changes in helper fun…

99e78c4

…ctions

gregmaya added 2 commits February 13, 2023 12:59

formatting (1)

5a3109e

updating ASSERTs in tests

392750d

jGaboardi added GSoC 2022 preprocessing labels Feb 13, 2023

gregmaya marked this pull request as ready for review February 13, 2023 16:01

gregmaya marked this pull request as draft February 16, 2023 10:12

gregmaya added 4 commits February 16, 2023 11:17

replacing hausdodorff distance with max distance to centroid

997db14

formatting (the wright way)

88e7e0b

fixing np.linalg.norm use

f84f9d4

assert update on new test for low diameter_factor

62eab1f

gregmaya marked this pull request as ready for review February 16, 2023 11:41

martinfleis reviewed Mar 6, 2023

View reviewed changes

updating code based on previous suggestions

6cc8b8f

add esda to CI and import it within fn

7548af0

martinfleis closed this Mar 30, 2023

martinfleis reopened this Mar 30, 2023

Merge remote-tracking branch 'upstream/main' into pr/gregmaya/465

f1d67a2

martinfleis reviewed Mar 30, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: helper functions part 1 #465

ENH: helper functions part 1 #465

gregmaya commented Feb 1, 2023

codecov bot commented Feb 2, 2023 •

edited

gregmaya commented Feb 2, 2023

martinfleis commented Feb 2, 2023

gregmaya commented Feb 2, 2023

gregmaya commented Feb 8, 2023

martinfleis commented Feb 13, 2023

gregmaya commented Feb 13, 2023 •

edited

jGaboardi commented Feb 13, 2023

jGaboardi commented Feb 13, 2023

gregmaya commented Feb 13, 2023

jGaboardi commented Feb 13, 2023

gregmaya commented Feb 15, 2023

martinfleis commented Feb 15, 2023

gregmaya commented Feb 23, 2023

martinfleis commented Feb 23, 2023

martinfleis left a comment

martinfleis Mar 6, 2023

gregmaya Mar 16, 2023

martinfleis Mar 6, 2023

martinfleis Mar 6, 2023

gregmaya Mar 16, 2023

gregmaya commented Mar 15, 2023

gregmaya commented Mar 16, 2023

gregmaya commented Mar 21, 2023

martinfleis commented Mar 21, 2023

gregmaya commented Mar 29, 2023

martinfleis commented Mar 30, 2023

martinfleis left a comment

martinfleis Mar 30, 2023

martinfleis Mar 30, 2023

martinfleis commented Apr 8, 2023

gregmaya commented Apr 8, 2023

		@@ -756,6 +757,57 @@ def _polygonize_ifnone(edges, polys):
		return polys


		def _create_shape_index(gdf):

ENH: helper functions part 1 #465

Are you sure you want to change the base?

ENH: helper functions part 1 #465

Conversation

gregmaya commented Feb 1, 2023

codecov bot commented Feb 2, 2023 • edited

Codecov Report

gregmaya commented Feb 2, 2023

martinfleis commented Feb 2, 2023

gregmaya commented Feb 2, 2023

gregmaya commented Feb 8, 2023

martinfleis commented Feb 13, 2023

gregmaya commented Feb 13, 2023 • edited

jGaboardi commented Feb 13, 2023

jGaboardi commented Feb 13, 2023

gregmaya commented Feb 13, 2023

jGaboardi commented Feb 13, 2023

gregmaya commented Feb 15, 2023

martinfleis commented Feb 15, 2023

gregmaya commented Feb 23, 2023

martinfleis commented Feb 23, 2023

martinfleis left a comment

Choose a reason for hiding this comment

martinfleis Mar 6, 2023

Choose a reason for hiding this comment

gregmaya Mar 16, 2023

Choose a reason for hiding this comment

martinfleis Mar 6, 2023

Choose a reason for hiding this comment

martinfleis Mar 6, 2023

Choose a reason for hiding this comment

gregmaya Mar 16, 2023

Choose a reason for hiding this comment

gregmaya commented Mar 15, 2023

gregmaya commented Mar 16, 2023

gregmaya commented Mar 21, 2023

martinfleis commented Mar 21, 2023

gregmaya commented Mar 29, 2023

martinfleis commented Mar 30, 2023

martinfleis left a comment

Choose a reason for hiding this comment

martinfleis Mar 30, 2023

Choose a reason for hiding this comment

martinfleis Mar 30, 2023

Choose a reason for hiding this comment

martinfleis commented Apr 8, 2023

gregmaya commented Apr 8, 2023

codecov bot commented Feb 2, 2023 •

edited

gregmaya commented Feb 13, 2023 •

edited