-
Notifications
You must be signed in to change notification settings - Fork 58
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
Tess simplification #595
base: main
Are you sure you want to change the base?
Tess simplification #595
Conversation
momepy/functional/_elements.py
Outdated
@@ -15,6 +15,7 @@ | |||
GPD_GE_013 = Version(gpd.__version__) >= Version("0.13.0") | |||
GPD_GE_10 = Version(gpd.__version__) >= Version("1.0dev") | |||
LPS_GE_411 = Version(libpysal.__version__) >= Version("4.11.dev") | |||
SHPLY_GE_205 = Version(shapely.__version__) >= Version("2.0.5") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shall check for shapely 2.1. There may be 2.0.5 or 2.0.6 none of which will include it. Shapely in the dev CI env reports shapely-2.1.0.dev0+83.ge9be991
momepy/functional/_elements.py
Outdated
from shapely import coverage_simplify | ||
|
||
##simplify singles here and let the threads simplify the actual tesselations | ||
simplified = coverage_simplify(enclosures.iloc[single], 1e-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, we should not touch enclosures. They are not problematic and this will result in mismatch with streets.
momepy/functional/_elements.py
Outdated
@@ -252,9 +271,14 @@ def _tess(ix, poly, blg, threshold, shrink, segment, enclosure_id): | |||
return_input=False, | |||
as_gdf=True, | |||
) | |||
if to_simplify: | |||
tess.geometry = coverage_simplify(tess.geometry, 1e-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tess.geometry = coverage_simplify(tess.geometry, 1e-1) | |
tess.geometry = coverage_simplify(tess.geometry, 1e-1, simplify_boundary=False) |
Otherwise you break contiguity and match with streets
momepy/functional/_elements.py
Outdated
if to_simplify: | ||
poly = coverage_simplify(poly, 1e-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #595 +/- ##
=======================================
+ Coverage 97.4% 97.5% +0.1%
=======================================
Files 26 38 +12
Lines 4328 6341 +2013
=======================================
+ Hits 4214 6180 +1966
- Misses 114 161 +47
|
momepy/functional/_elements.py
Outdated
|
||
if len(blg) > 1: | ||
if len(blg) >= 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if else please
momepy/functional/_elements.py
Outdated
from shapely import coverage_simplify | ||
|
||
tess.geometry = coverage_simplify( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from shapely import coverage_simplify | |
tess.geometry = coverage_simplify( | |
tess.geometry = shapely.coverage_simplify( |
We do import shapely on top
Please split simplification and the fix into two distinct PRs. The former will wait until we get |
Adding the option to simplify the enclosed tessellations & a filtering fix