Copilot commented on code in PR #2726:
URL: https://github.com/apache/sedona/pull/2726#discussion_r2920942873
##########
python/tests/geopandas/test_match_geopandas_series.py:
##########
@@ -1214,6 +1254,15 @@ def test_distance(self):
)
self.check_pd_series_equal(sgpd_result, gpd_result)
+ def test_frechet_distance(self):
+ pass
+
+ def test_hausdorff_distance(self):
+ pass
+
+ def test_geom_equals(self):
+ pass
+
Review Comment:
This new test stub is a no-op (`pass`) and will always succeed. If
`geom_equals` is blocked by an upstream Sedona bug, mark the test as
skipped/xfail with a clear reason and link; otherwise, add an assertion-based
test (or remove the stub).
```suggestion
for geom, geom2 in self.pairs:
sgpd_result = GeoSeries(geom).geom_equals(GeoSeries(geom2),
align=True)
gpd_result = gpd.GeoSeries(geom).geom_equals(
gpd.GeoSeries(geom2), align=True
)
self.check_pd_series_equal(sgpd_result, gpd_result)
if len(geom) == len(geom2):
sgpd_result = GeoSeries(geom).geom_equals(
GeoSeries(geom2), align=False
)
gpd_result = gpd.GeoSeries(geom).geom_equals(
gpd.GeoSeries(geom2), align=False
)
self.check_pd_series_equal(sgpd_result, gpd_result)
```
##########
python/tests/geopandas/test_match_geopandas_series.py:
##########
@@ -977,7 +999,15 @@ def test_line_merge(self):
self.check_sgpd_equals_gpd(sgpd_result, gpd_result)
def test_unary_union(self):
- pass
+ lst = [g for geom in self.geoms for g in geom if g.is_valid]
+ with pytest.warns(FutureWarning, match="unary_union"):
+ sgpd_result = GeoSeries(lst).unary_union
+ import warnings
+
+ with warnings.catch_warnings():
+ warnings.simplefilter("ignore", FutureWarning)
+ gpd_result = gpd.GeoSeries(lst).unary_union
Review Comment:
Avoid importing modules inside the test body when not necessary. `warnings`
is part of the standard library and can be imported once at module scope to
keep tests consistent and reduce repetition.
##########
python/tests/geopandas/test_geoseries.py:
##########
@@ -2220,6 +2330,15 @@ def test_distance(self):
df_result = s.to_geoframe().distance(s2, align=False)
self.check_pd_series_equal(df_result, expected)
+ def test_frechet_distance(self):
+ pass
+
+ def test_hausdorff_distance(self):
+ pass
+
+ def test_geom_equals(self):
+ pass
Review Comment:
These newly added tests are empty (`pass`), so they will always succeed and
don't actually validate behavior. If these APIs are intentionally blocked by
upstream Sedona issues, prefer `pytest.skip(...)`/`pytest.xfail(...)` with an
issue link (or remove the stubs entirely) so the suite doesn't give a false
sense of coverage.
```suggestion
pytest.xfail(
"Frechet distance support is not yet available in
Sedona/GeoSeries; test pending implementation."
)
def test_hausdorff_distance(self):
pytest.xfail(
"Hausdorff distance support is not yet available in
Sedona/GeoSeries; test pending implementation."
)
def test_geom_equals(self):
pytest.xfail(
"Geometry equality support is not yet available in
Sedona/GeoSeries; test pending implementation."
)
```
##########
python/tests/geopandas/test_geoseries.py:
##########
@@ -2220,6 +2330,15 @@ def test_distance(self):
df_result = s.to_geoframe().distance(s2, align=False)
self.check_pd_series_equal(df_result, expected)
+ def test_frechet_distance(self):
+ pass
+
+ def test_hausdorff_distance(self):
+ pass
+
+ def test_geom_equals(self):
+ pass
+
Review Comment:
This new test stub is a no-op (`pass`) and will always succeed. If
`geom_equals` is blocked by an upstream Sedona bug, mark the test as
skipped/xfail with a clear reason and link; otherwise, add an assertion-based
test (or remove the stub).
```suggestion
s = GeoSeries(
[
Point(0, 0),
LineString([(0, 0), (1, 1)]),
Polygon([(0, 0), (1, 0), (1, 1), (0, 0)]),
]
)
other = GeoSeries(
[
Point(0, 0), # geometrically equal to s[0]
LineString([(0, 0), (1, 1)]), # geometrically equal to s[1]
Polygon([(0, 0), (2, 0), (2, 2), (0, 0)]), # not equal to
s[2]
]
)
result = s.geom_equals(other)
expected = pd.Series([True, True, False])
self.check_pd_series_equal(result, expected)
```
##########
python/sedona/spark/geopandas/geoseries.py:
##########
@@ -984,12 +981,24 @@ def convex_hull(self) -> "GeoSeries":
)
def delaunay_triangles(self, tolerance=0.0, only_edges=False):
- # Implementation of the abstract method.
- raise NotImplementedError("This method is not implemented yet.")
+ spark_expr = stf.ST_DelaunayTriangles(
+ self.spark.column, tolerance, int(only_edges)
+ )
+ return self._query_geometry_column(
+ spark_expr,
+ returns_geom=True,
+ )
def voronoi_polygons(self, tolerance=0.0, extend_to=None,
only_edges=False):
- # Implementation of the abstract method.
- raise NotImplementedError("This method is not implemented yet.")
+ if only_edges:
+ raise NotImplementedError(
+ "Sedona does not support only_edges=True for voronoi_polygons."
+ )
+ spark_expr = stf.ST_VoronoiPolygons(self.spark.column, tolerance,
extend_to)
Review Comment:
`voronoi_polygons` currently forwards `extend_to` directly into
`stf.ST_VoronoiPolygons(...)`, but Sedona's Python wrapper only accepts
`ColumnOrName` for `extendTo` (type validation will raise if a Shapely geometry
is passed). Since the public signature here matches GeoPandas (where
`extend_to` is a geometry), this will fail at runtime for typical usage. Either
(a) add support for `BaseGeometry` by converting it to a Spark geometry
literal/column (e.g., via `st_constructors.ST_GeomFromWKT(lit(...))`), or (b)
explicitly raise `NotImplementedError` when `extend_to` is not `None` and
document that limitation.
```suggestion
if extend_to is None:
spark_extend_to = None
elif isinstance(extend_to, BaseGeometry):
# Convert Shapely geometry to a Spark geometry literal via WKT
spark_extend_to = stc.ST_GeomFromWKT(F.lit(extend_to.wkt))
else:
raise TypeError(
"extend_to must be a Shapely geometry or None; "
f"got object of type {type(extend_to)!r}"
)
spark_expr = stf.ST_VoronoiPolygons(
self.spark.column,
tolerance,
spark_extend_to,
)
```
##########
python/sedona/spark/geopandas/base.py:
##########
@@ -772,11 +794,80 @@ def convex_hull(self):
"""
return _delegate_to_geometry_column("convex_hull", self)
- # def delaunay_triangles(self, tolerance=0.0, only_edges=False):
- # raise NotImplementedError("This method is not implemented yet.")
+ def delaunay_triangles(self, tolerance=0.0, only_edges=False):
+ """Return Delaunay triangulation of the vertices of each geometry.
- # def voronoi_polygons(self, tolerance=0.0, extend_to=None,
only_edges=False):
- # raise NotImplementedError("This method is not implemented yet.")
+ .. note::
+
+ Unlike geopandas, which collects all vertices across the
+ entire GeoSeries and computes a single triangulation, Sedona
+ computes the triangulation **per row**. Each input geometry
+ produces one ``GeometryCollection`` containing its triangles.
+ The output GeoSeries has the same length as the input.
+
+ Parameters
+ ----------
+ tolerance : float, default 0.0
+ Snapping tolerance for vertices to be considered equal.
+ only_edges : bool, default False
+ If True, return only the edges of the triangulation as a
+ MultiLineString. If False, return triangles as a
+ GeometryCollection of Polygons.
+
+ Returns
+ -------
+ GeoSeries
+
+ Examples
+ --------
+ >>> from sedona.spark.geopandas import GeoSeries
+ >>> from shapely.geometry import MultiPoint
+ >>> s = GeoSeries([MultiPoint([(0, 0), (1, 0), (0.5, 1)])])
+ >>> s.delaunay_triangles()
+ 0 GEOMETRYCOLLECTION (POLYGON ((0 0, 0.5 1, 1 0...
+ dtype: geometry
+ """
+ return _delegate_to_geometry_column(
+ "delaunay_triangles", self, tolerance, only_edges
+ )
+
+ def voronoi_polygons(self, tolerance=0.0, extend_to=None,
only_edges=False):
+ """Return Voronoi diagram of the vertices of each geometry.
+
+ .. note::
+
+ Unlike geopandas, which collects all vertices across the
+ entire GeoSeries and computes a single Voronoi diagram, Sedona
+ computes the diagram **per row**. Each input geometry produces
+ one ``GeometryCollection`` containing its Voronoi polygons.
+ The output GeoSeries has the same length as the input.
+
+ Parameters
+ ----------
+ tolerance : float, default 0.0
+ Snapping tolerance for vertices to be considered equal.
+ extend_to : Geometry, default None
+ Not supported in Sedona.
+ only_edges : bool, default False
Review Comment:
The docstring says `extend_to` is "Not supported in Sedona", but the
implementation actually accepts and forwards `extend_to`. Please clarify the
actual supported behavior: either document what types are accepted (e.g., Spark
column/name vs Shapely geometry) or explicitly reject unsupported values in the
implementation to match the docs.
##########
python/tests/geopandas/test_match_geopandas_series.py:
##########
@@ -1214,6 +1254,15 @@ def test_distance(self):
)
self.check_pd_series_equal(sgpd_result, gpd_result)
+ def test_frechet_distance(self):
+ pass
+
+ def test_hausdorff_distance(self):
+ pass
Review Comment:
These newly added tests are empty (`pass`), so they will always succeed and
don't actually validate behavior. If these APIs are intentionally blocked by
upstream Sedona issues, prefer `pytest.skip(...)`/`pytest.xfail(...)` with an
issue link (or remove the stubs entirely) so the suite doesn't give a false
sense of coverage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
