Skip to content

FIX: support Polygon/MultiPolygon in Voronoi graph builder (#688)#868

Open
AliaaNasser7 wants to merge 11 commits into
pysal:mainfrom
AliaaNasser7:fix-voronoi-polygons
Open

FIX: support Polygon/MultiPolygon in Voronoi graph builder (#688)#868
AliaaNasser7 wants to merge 11 commits into
pysal:mainfrom
AliaaNasser7:fix-voronoi-polygons

Conversation

@AliaaNasser7

Copy link
Copy Markdown

Description

I've updated the _voronoi builder to support polygonal geometries by using their centroids as generators. This proposes a practical resolution for #688 and bypasses the limitations of the current point-based validation.

Changes & Technical Implementation

  • Validation Bypass: The @_validate_coplanar decorator was causing indexing issues and attribute errors with Polygons. I moved the coplanar check inside the function and used centroid extraction to ensure the math stays consistent with the Voronoi logic.
  • Handling Geometry Types: I added a check for .centroid to handle GeoDataFrames directly. For other inputs, I'm using get_points_array with a fallback to reshape the array, ensuring we always pass an (n, 2) array to voronoi_frames.
  • Ghost Cell Filtering: Clipping often creates auxiliary cells that mess up the input-to-output mapping. I fixed this by explicitly mapping the heads_ix and tails_ix back to the original ids before returning the graph.
  • Kwargs: Passed **kwargs down to voronoi_frames to support parameters like shrink or buffer.

Verification

I tested this with a GeoDataFrame of adjacent polygons. The builder now correctly identifies neighbors and maintains the original index without crashing or throwing indexing errors.

Related to #688

@sjsrey

sjsrey commented Feb 6, 2026

Copy link
Copy Markdown
Member

Thank you for this, but the changes are breaking the tests.
Please ensure the tests are passing prior to requesting a review of the PR.

@AliaaNasser7

Copy link
Copy Markdown
Author

Thank you for this, but the changes are breaking the tests. Please ensure the tests are passing prior to requesting a review of the PR.

Thanks for the feedback! I’m working through the failing tests.
I saw this is labeled as a good first issue, just wanted to check if you think it’s still a good starting point.

@codecov

codecov Bot commented Feb 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.5%. Comparing base (7304239) to head (35f2e72).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #868   +/-   ##
=====================================
  Coverage   85.5%   85.5%           
=====================================
  Files        151     151           
  Lines      16092   16141   +49     
=====================================
+ Hits       13758   13807   +49     
  Misses      2334    2334           
Files with missing lines Coverage Δ
libpysal/graph/_triangulation.py 98.2% <100.0%> (+0.1%) ⬆️
libpysal/graph/base.py 96.6% <100.0%> (+<0.1%) ⬆️
libpysal/graph/tests/test_triangulation.py 98.9% <100.0%> (+0.1%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AliaaNasser7

AliaaNasser7 commented Feb 9, 2026

Copy link
Copy Markdown
Author

Thank you for this, but the changes are breaking the tests. Please ensure the tests are passing prior to requesting a review of the PR.
The tests are now passing successfully. I fixed the issues that were breaking the tests earlier. Ready for review.

@martinfleis martinfleis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This completely missed the point of #688. Have a look at #678 which it refers to and try to understand what the aim was there. This PR does not support Polygons in Voronoi builders, it silently downcasts them to centroids.

@AliaaNasser7

AliaaNasser7 commented Feb 20, 2026

Copy link
Copy Markdown
Author

This completely missed the point of #688. Have a look at #678 which it refers to and try to understand what the aim was there. This PR does not support Polygons in Voronoi builders, it silently downcasts them to centroids.

@martinfleis I reworked the PR completely ,instead of downcasting to centroids, _voronoi now passes geometries directly to voronoi_frames (which handles all geometry types since #678) and forwards segment/shrink kwargs.

@martinfleis martinfleis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be easier to keep the current _voronoi function intact with its coplanarity solutions and write a separate one for non-point geometry types. And just channel to the proper one when needed.

Comment thread libpysal/graph/_triangulation.py Outdated
Comment on lines +435 to +440
if coplanar == "jitter":
coords = shapely.get_coordinates(geoms)
coords = _jitter_geoms(coords, seed=seed)
geoms = geopandas.GeoSeries(
shapely.points(coords),
index=geoms.index if hasattr(geoms, "index") else None,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to get very wrong when non-point geometry is given.

Please ensure that you write test suite alongside your contributions. Many of the issues you'd be able to catch yourself.

@AliaaNasser7

Copy link
Copy Markdown
Author

It might be easier to keep the current _voronoi function intact with its coplanarity solutions and write a separate one for non-point geometry types. And just channel to the proper one when needed.

Thank you for the guidance! ok, I will do the changes and also add proper tests this time.

@AliaaNasser7

AliaaNasser7 commented Mar 3, 2026

Copy link
Copy Markdown
Author

It might be easier to keep the current _voronoi function intact with its coplanarity solutions and write a separate one for non-point geometry types. And just channel to the proper one when needed.

@martinfleis I reworked based on your suggestion, kept _voronoi intact and added a separate _voronoi_polygon for non-point geometries, build_triangulation now routes to the appropriate function based on geometry type.

Comment on lines +514 to +515
heads, tails, weights = _voronoi_polygon(gdf, ids=ids)
assert len(np.unique(heads)) == 4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests should look like those above. Check the exact arrays, all of them.


@pytest.mark.network
def test_coplanar_jitter_voronoi(stores, stores_unique):
def test_coplanar_jitter_voronoi(stores, stores_unique): # edit

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def test_coplanar_jitter_voronoi(stores, stores_unique): # edit
def test_coplanar_jitter_voronoi(stores, stores_unique):

Comment on lines 431 to +434
#### utilities


def _voronoi_polygon(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be under "utilities" but above.

Comment on lines +486 to +489
heads = heads_ix
tails = tails_ix
weights = numpy.ones(len(heads_ix), dtype=numpy.int8)
return heads, tails, weights

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
heads = heads_ix
tails = tails_ix
weights = numpy.ones(len(heads_ix), dtype=numpy.int8)
return heads, tails, weights
weights = numpy.ones(len(heads_ix), dtype=numpy.int8)
return heads_ix, tails_ix, weights

Comment thread libpysal/graph/base.py
Comment on lines +1453 to +1454
``method="voronoi"``. Supports ``segment`` (float) and ``shrink``
(float). Ignored for other methods.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for non-point inputs.

Comment thread libpysal/graph/base.py

Parameters
----------
data : numpy.ndarray, geopandas.GeoSeries, geopandas.GeoDataFrame

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be updated to also mention other geometry types.

Comment thread libpysal/graph/base.py
decay=decay,
taper=taper,
)
if hasattr(data, "geom_type") and not set(data.geom_type) <= {"Point"}:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use shapely.get_type_id in here to check the geometry types for all inputs, not just geopandas ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants