petern48 commented on code in PR #2512:
URL: https://github.com/apache/sedona/pull/2512#discussion_r2544494065
##########
python/sedona/spark/geopandas/base.py:
##########
@@ -949,8 +949,68 @@ def force_2d(self):
"""
return _delegate_to_geometry_column("force_2d", self)
- # def force_3d(self, z=0):
- # raise NotImplementedError("This method is not implemented yet.")
+ def force_3d(self, z=0):
+ """Force the dimensionality of a geometry to 3D.
+
+ 2D geometries will get the provided Z coordinate; 3D geometries
+ are unchanged (unless their Z coordinate is ``np.nan``).
Review Comment:
Can you add a test for this in `test_geoseries.py`? I see that this is from
the original geopandas docstring, so I'm not entirely sure if we follow the
same behavior naturally in Sedona. If we don't let me know, and we can figure
out what to do next.
We could test something like whether `Point(1, 1, np.nan)` becomes
`Point(1, 1, 3)` when calling `.force_3d(3)`
##########
python/sedona/spark/geopandas/base.py:
##########
@@ -949,8 +949,68 @@ def force_2d(self):
"""
return _delegate_to_geometry_column("force_2d", self)
- # def force_3d(self, z=0):
- # raise NotImplementedError("This method is not implemented yet.")
+ def force_3d(self, z=0):
+ """Force the dimensionality of a geometry to 3D.
+
+ 2D geometries will get the provided Z coordinate; 3D geometries
+ are unchanged (unless their Z coordinate is ``np.nan``).
+
+ Note that for empty geometries, 3D is only supported since GEOS 3.9
and then
+ still only for simple geometries (non-collections).
+
Review Comment:
```suggestion
```
I think we can delete this part. The `GEOS 3.9` part doesn't apply to use
because we're built not on [GEOS](https://github.com/libgeos/geos) (which is a
C library). And I think we do support collections since the
`test_match_geopandas_series` test passed naturally (`self.geoms` in that test
contains collections). Could you add a single test in `test_geoseries.py` that
tests a GEOMETRYCOLLECTION with a non-zero z-arg?
e.g. `GeometryCollection(Point(1, 1), LineString([(1, 1), (0, 1), (1, 0)]))`
and `force_3d(3)`
##########
python/tests/geopandas/test_match_geopandas_series.py:
##########
@@ -878,7 +878,50 @@ def test_force_2d(self):
self.check_sgpd_equals_gpd(sgpd_3d, gpd_3d)
def test_force_3d(self):
- pass
+ # force_3d was added from geopandas 1.0.0
+ if parse_version(gpd.__version__) < parse_version("1.0.0"):
+ pytest.skip("geopandas force_3d requires version 1.0.0 or higher")
+ # 1) Promote 2D to 3D with default z=0.0
+ for geom in self.geoms:
+ sgpd_result = GeoSeries(geom).force_3d()
+ gpd_result = gpd.GeoSeries(geom).force_3d()
Review Comment:
```suggestion
sgpd_result = GeoSeries(geom).force_3d(4)
gpd_result = gpd.GeoSeries(geom).force_3d(4)
```
This is a minor change, and I don't think Sedona would fail it, but I think
this would be a slightly better test if we test a non-zero z argument. These
"match" tests in `test_match_geopandas_series.py` are the most
comprehensive/powerful, so I'd prefer to avoid a common default case like 0.
e.g. the chances that we have a bug that always set z=0 in some case is much
higher than the chance that it always set z=4.
##########
python/sedona/spark/geopandas/base.py:
##########
@@ -949,8 +949,68 @@ def force_2d(self):
"""
return _delegate_to_geometry_column("force_2d", self)
- # def force_3d(self, z=0):
- # raise NotImplementedError("This method is not implemented yet.")
+ def force_3d(self, z=0):
+ """Force the dimensionality of a geometry to 3D.
+
+ 2D geometries will get the provided Z coordinate; 3D geometries
+ are unchanged (unless their Z coordinate is ``np.nan``).
+
+ Note that for empty geometries, 3D is only supported since GEOS 3.9
and then
+ still only for simple geometries (non-collections).
+
+ Parameters
+ ----------
+ z : float | array_like (default 0)
+ Z coordinate to be assigned
+
+ Returns
+ -------
+ GeoSeries
+
+ Examples
+ --------
+ >>> from shapely import Polygon, LineString, Point
+ >>> s = geopandas.GeoSeries(
Review Comment:
```suggestion
>>> from shapely import Polygon, LineString, Point
>>> from sedona.spark.geopandas import GeoSeries
>>> s = GeoSeries(
```
nit
--
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]
