petern48 commented on code in PR #2502:
URL: https://github.com/apache/sedona/pull/2502#discussion_r2525554107
##########
python/tests/geopandas/test_match_geopandas_series.py:
##########
@@ -781,7 +781,15 @@ def test_minimum_bounding_circle(self):
self.check_sgpd_equals_gpd(sgpd_result, gpd_result, tolerance=0.5)
def test_minimum_bounding_radius(self):
- pass
+ # minimum_bounding_radius was added from geopandas 1.0.0 and later
+ if parse_version(gpd.__version__) < parse_version("1.0.0"):
+ pytest.skip(
+ "geopandas minimum_bounding_radius requires version 1.0.0 or
higher"
+ )
Review Comment:
```suggestion
```
It was added much earlier, in version 0.13: See
[here](https://geopandas.org/en/stable/docs/changelog.html#:~:text=Added%20minimum_bounding_radius()%20as%20GeoSeries%20method%20(%232827).).
Avoid blindly copying `pytest.skips`. We should avoid them by default and only
include them if necessary to pass tests (and also for a good reason).
##########
python/tests/geopandas/test_geoseries.py:
##########
@@ -1309,7 +1309,30 @@ def test_minimum_bounding_circle(self):
self.check_sgpd_equals_gpd(df_result, expected)
def test_minimum_bounding_radius(self):
- pass
+ s = GeoSeries(
+ [
+ Polygon([(0, 0), (1, 0), (1, 1), (0, 1)]),
+ LineString([(0, 0), (2, 0)]),
+ Point(0, 0),
+ None,
+ ]
+ )
+
+ expected = gpd.GeoSeries(
+ [
+ Polygon([(0, 0), (1, 0), (1, 1), (0, 1)]),
+ LineString([(0, 0), (2, 0)]),
+ Point(0, 0),
+ None,
+ ]
+ ).minimum_bounding_radius()
Review Comment:
This should be a `pd.Series([])` of the floats.
`test_geoseries.py` is for hard-coding the expected output.
`test_match_geopandas_series.py` is for comparing if the result is the same
as the original geopandas, which is what you did here.
Also slight preference that we use the examples in the doc string in
`base.py`.
##########
python/sedona/spark/geopandas/base.py:
##########
@@ -742,8 +742,39 @@ def minimum_bounding_circle(self):
"""
return _delegate_to_geometry_column("minimum_bounding_circle", self)
- # def minimum_bounding_radius(self):
- # raise NotImplementedError("This method is not implemented yet.")
+ def minimum_bounding_radius(self):
+ """
+ Returns a ``Series`` containing the radius of the minimum bounding
circle
+ of each geometry.
+
+ The minimum bounding circle is the smallest circle that completely
encloses
+ the geometry. This method returns only the radius of that circle,
expressed
+ in the units of the GeoSeries' coordinate reference system.
+
+ Returns
+ -------
+ Series
+ A Series containing the radius of the minimum bounding circle
+ for each geometry.
+
+ Examples
+ --------
+ >>> from shapely.geometry import Point, Polygon, LineString
+ >>> from sedona.spark.geopandas import GeoSeries
+ >>> s = GeoSeries(
+ ... [
+ ... Point(0, 0),
+ ... LineString([(0, 0), (1, 1)]),
+ ... Polygon([(0, 0), (3, 0), (3, 3), (0, 3)]),
+ ... ]
+ ... )
+ >>> s.minimum_bounding_radius()
+ 0 0.000000
+ 1 ...
+ 2 ...
Review Comment:
We can't just say the answer is "`...`". The whole point of the docstring is
to explain what the behavior is. We should be **copy-pasting the docstring
directly from geopandas** (and then making small tweaks to make it import
sedona instead of geopandas), as mentioned in instruction 4 of the [epic
issue](https://github.com/apache/sedona/issues/2230).
Here's the corresponding docstring in geopandas:
https://github.com/geopandas/geopandas/blob/503ace4b76c36a9b46fd9b9f85cd68ca046f1376/geopandas/base.py#L1917-L1947
If these were AI-generated, we can't rely on AI to generate the correct
docstrings bc sometimes geopandas' behavior is non-intuitive, and LLMs won't
get that. Please do also fix the docstrings for the other functions you
implemented (in this PR is fine). Most of the docstrings should all be in that
same
[base.py](https://github.com/geopandas/geopandas/blob/503ace4b76c36a9b46fd9b9f85cd68ca046f1376/geopandas/base.py)
file in the geopandas repo.
--
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]
