petern48 commented on code in PR #2772:
URL: https://github.com/apache/sedona/pull/2772#discussion_r2978498887
##########
python/tests/geopandas/test_geoseries.py:
##########
@@ -1614,7 +1614,19 @@ def test_minimum_bounding_radius(self):
self.check_pd_series_equal(df_result, expected)
def test_minimum_clearance(self):
- pass
+ s = GeoSeries(
+ [
+ Polygon([(0, 0), (1, 0), (1, 1), (0, 1)]),
+ Polygon([(0, 0), (0.5, 0), (0.5, 0.5), (0, 0.5)]),
+ ]
+ )
+ expected = pd.Series([1.0, 0.5])
Review Comment:
```suggestion
Polygon([(0, 0), (0.5, 0), (0.5, 0.5), (0, 0.5)]),
MultiPoint([(1, 1), (1, 1)]),
]
)
expected = pd.Series([1.0, 0.5, float("inf")])
```
I'd like to add this edge case, since it's not covered here or in
`test_match_geopandas_series.py`. MultiPoint with identical points should
return an inf, as Point would. Tested this locally, and already confirmed this
test should pass.
##########
python/sedona/spark/geopandas/base.py:
##########
@@ -489,7 +489,6 @@ def is_ring(self):
"""
return _delegate_to_geometry_column("is_ring", self)
- # @property
# def is_ccw(self):
Review Comment:
```suggestion
# @property
# def is_ccw(self):
```
Let's undo this deletion too, since we're doing one more iteration anyways.
##########
python/sedona/spark/geopandas/geoseries.py:
##########
@@ -1081,9 +1078,14 @@ def minimum_bounding_radius(self) -> pspd.Series:
returns_geom=False,
)
- def minimum_clearance(self):
- # Implementation of the abstract method.
- raise NotImplementedError("This method is not implemented yet.")
+ def minimum_clearance(self) -> pspd.Series:
+ spark_col = stf.ST_MinimumClearance(self.spark.column)
+ # JTS returns Double.MAX_VALUE for degenerate geometries (e.g. Point,
empty);
+ # convert to float('inf') to match geopandas/shapely behaviour.
+ spark_expr = F.when(
+ spark_col >= sys.float_info.max, F.lit(float("inf"))
+ ).otherwise(spark_col)
Review Comment:
I checked out this code and confirmed this extra conditional logic is
necessary to pass tests 👍
(Just a note, no action needed by you)
##########
python/sedona/spark/geopandas/base.py:
##########
@@ -1129,8 +1128,33 @@ def minimum_bounding_radius(self):
"""
return _delegate_to_geometry_column("minimum_bounding_radius", self)
- # def minimum_clearance(self):
- # raise NotImplementedError("This method is not implemented yet.")
+ def minimum_clearance(self) -> ps.Series:
+ """Return the minimum clearance of each geometry.
+
+ The minimum clearance is the smallest distance by which a vertex of
+ a geometry could be moved to produce an invalid geometry. A larger
+ value indicates a more robust geometry.
Review Comment:
Could you replace this entire docstring with a copy-paste from the original
geopandas one
[here](https://github.com/geopandas/geopandas/blob/8762d4899f8c111fd57ac5e652e091ded73bdbaf/geopandas/base.py#L1951-L1982)?
This is what we generally prefer to do. For this case, I like that their
docstring mentions the infinity edge case behavior, and the examples they give
are more helpful than the current ones that only show examples for polygons.
--
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]
