-
Notifications
You must be signed in to change notification settings - Fork 68
ENH: Added Cellular Automata-based Enclosed Tessellation #686
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #686 +/- ##
=======================================
+ Coverage 97.4% 98.3% +1.0%
=======================================
Files 26 26
Lines 4328 4397 +69
=======================================
+ Hits 4214 4323 +109
+ Misses 114 74 -40
🚀 New features to boost your workflow:
|
@yu-ta-sato This is really cool! Will you be able to add appropriate testing for this new functionality? |
@jGaboardi Yes of course! I wanted to be on the same page with you guys in advance, especially the direction of its implementation. If needed, I can separate the function from |
Hold on with testing, I'll have some requests but need more time to properly look at it. |
Hey, enclosed tessellation is supposed to be continuous coverage by definition. As illustrated below, that is not the case at the moment. Also, I would very much prefer to ensure that the lines are as straight as possible, without this artifact of the grid we see here. We can either do that in conversion of the grid to polygons or afterwards using coverage simplification. We will need to touch the API as well but let's focus on ensuring this behaves the way we want first. |
The cellular-automata grids are already polygonised in the returning outputs (by |
No. We need to ensure that whatever comes from the grid is continuous coverage and then use upcoming coverage_simplify which ensures that the simplification does not break topological relations between neighbouring cells. |
Ah, no. I did not. However, this should not be an option, it should be set by default. |
Need to chase the on-going discussion on shapely a bit more, but can we implement |
We can already do that. It will just be tested in the dev CI environment only. |
…pely < 2.1.0 raise an error
momepy/functional/_elements.py
Outdated
blg = blg[ | ||
shapely.area( | ||
shapely.intersection( | ||
blg.geometry.array, shapely.geometry.Polygon(poly.boundary) |
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.
What is the rationale behind this? Why are you ignoring holes when a MultiPolygon is provided? Feels inconsistent.
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.
The initial intention was to let the poly be accommodated with inner barriers, but now they're separated. Somehow this part remained, so will remove it.
momepy/functional/_elements.py
Outdated
if barrier_geoms.geom_type == "Polygon": | ||
# Take buffer of polygon and extract its exterior boundary | ||
barrier_geoms_buffered = GeoSeries( | ||
[barrier_geoms.buffer(10).exterior], crs=seed_geoms.crs |
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.
Where does 10 come from here? Can't use hardcoded values here.
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 amount of buffer was needed to ensure that all the extent are assigned to either of the cell state, which could be arbitrary number (This kind of vacant cells occurred between three tessellations). Will set the number based on the cell size automatically.
momepy/functional/_elements.py
Outdated
raise ImportError( | ||
"Shapely 2.1.0 or higher is required for 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.
We should raise this before we start doing anything else. Fail fast.
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.
Also, the error should say something like Shapely 2.1.0 or higher is required for tessellation with inner barriers provided.
momepy/functional/_elements.py
Outdated
# Only keep the geometry which is within the enclosure | ||
inner_barriers = inner_barriers[inner_barriers.within(enclosure)] | ||
|
||
return GeoSeries(inner_barriers.geometry, crs=barriers.crs) |
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.
Isn't inner_barriers
already a GeoSeries?
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.
I checked it, you're right.
Co-authored-by: Martin Fleischmann <[email protected]>
Hey, I pushed some changes and got this up to speed with main. The main issue I see now is performance. On the tiny bubenec dataset, this algorithm takes |
I am not excited about it, but since it is optional functionality maybe OK? |
@martinfleis @jGaboardi Thanks for the revisit and reminder! I agree with the concern about the notoriously huge computational order. Although the algorithm of Cell Automata is inebitably complex, I fixed some part of inefficiency as follows: Replaced per-cell polygon creation ( Before: used Now: uses In my local environment for bubenec dataset, now it's reduced like this: Before: 5.33 seconds Hope it works! |
Good job! @yu-ta-sato can you also try to craft some tests for this and expand the user guide on enclosed tessellation? You can add the test to the existing block. |
@martinfleis yes, I was about to work on them! |
Co-authored-by: James Gaboardi <[email protected]>
Co-authored-by: James Gaboardi <[email protected]>
That is probably something I caused in my commits but it is not immediately clear to me where. |
Tests and docstrings are done except linting. Tests are not covered for line 606 and 619 in |
@yu-ta-sato -- this is awesome!
I think these failures are actually happening due to gantt
dateFormat YYYY-MM-DD
axisFormat %m / %Y
title Support Window
section geopandas
0.13.0 : 2023-05-06,2025-05-05
0.14.0 : 2023-09-15,2025-09-14
1.0.0 : 2024-06-24,2026-06-24
1.1.0 : 2025-06-01,2027-06-01
|
Based on the discussion #684, I added Cellular Automata-based Voronoi diagram for
enclosed_tessellation
, remaining the API the same as before.Here's a sample code:
There're three patterns of enclosed tessellation with comparison:
In the Liverpool City Region by the Overture Maps, the output of the new algorithm with inner barriers (cell size: 1m)are like this:

If it's good to go, let me update the docs.