Copilot commented on code in PR #2726:
URL: https://github.com/apache/sedona/pull/2726#discussion_r2921253392
##########
python/sedona/spark/geopandas/geoseries.py:
##########
@@ -1203,8 +1222,17 @@ def crosses(self, other, align=None) -> pspd.Series:
return _to_bool(result)
def disjoint(self, other, align=None):
Review Comment:
`disjoint` is a boolean predicate like `crosses`/`intersects`, but it
currently lacks a return type annotation. Adding `-> pspd.Series` would keep
type hints consistent with the other predicate methods in this class.
```suggestion
def disjoint(self, other, align=None) -> pspd.Series:
```
##########
python/sedona/spark/geopandas/geoseries.py:
##########
@@ -984,12 +981,28 @@ 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)
+ )
Review Comment:
`delaunay_triangles` accepts `only_edges=True`, but the tests only cover the
default (`False`). Adding a unit test that exercises `only_edges=True` (and
asserts the returned geometry type, e.g., MultiLineString) would prevent
regressions in that code path.
##########
python/sedona/spark/geopandas/base.py:
##########
@@ -1337,9 +1429,35 @@ def line_merge(self, directed=False):
"""
return _delegate_to_geometry_column("line_merge", self, directed)
- # @property
- # def unary_union(self):
- # raise NotImplementedError("This method is not implemented yet.")
+ @property
+ def unary_union(self):
+ """Returns a geometry containing the union of all geometries in the
+ ``GeoSeries``.
+
+ .. deprecated::
+ The ``unary_union`` attribute is deprecated. Use
+ :meth:`union_all` instead.
Review Comment:
The `.. deprecated::` directive requires a version argument in
Sphinx/docutils; leaving it blank can cause doc build warnings/errors. Consider
adding an explicit version (e.g., `.. deprecated:: <version>`) or replacing the
directive with plain-text deprecation wording.
```suggestion
Deprecated: The ``unary_union`` attribute is deprecated. Use
:meth:`union_all` instead.
```
##########
python/sedona/spark/geopandas/base.py:
##########
@@ -1502,6 +1620,36 @@ def crosses(self, other, align=None) -> ps.Series:
"""
return _delegate_to_geometry_column("crosses", self, other, align)
+ def disjoint(self, other, align=None):
+ """Returns a ``Series`` of ``dtype('bool')`` with value ``True`` for
+ each aligned geometry that is disjoint from `other`.
+
+ An object is said to be disjoint from `other` if its
+ `boundary` and `interior` does not intersect at all with those of the
+ other.
Review Comment:
Grammar in the `disjoint` docstring: "boundary and interior does not
intersect" should be "boundary and interior do not intersect" (or rephrase to
avoid subject/verb disagreement).
```suggestion
`boundary` and `interior` do not intersect at all with those of the
other object.
```
##########
python/sedona/spark/geopandas/geoseries.py:
##########
@@ -984,12 +981,28 @@ 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."
+ )
+ if extend_to is not None:
+ raise NotImplementedError(
+ "Sedona does not support extend_to for voronoi_polygons."
+ )
Review Comment:
`voronoi_polygons` has an explicit `extend_to is not None` branch that
raises `NotImplementedError`, but there is no test asserting that behavior.
Adding a small unit test that passes a non-None `extend_to` and checks for
`NotImplementedError` would cover this new branch.
--
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]
