You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "petern48 (via GitHub)" <gi...@apache.org> on 2025/09/05 18:28:47 UTC
[PR] [GH-2331] Geopandas: Document differences of sindex compared to gpd + sindex fixes [sedona]
petern48 opened a new pull request, #2332:
URL: https://github.com/apache/sedona/pull/2332
## Did you read the Contributor Guide?
- Yes, I have read the [Contributor Rules](https://sedona.apache.org/latest/community/rule/) and [Contributor Development Guide](https://sedona.apache.org/latest/community/develop/)
## Is this PR related to a ticket?
- Yes, and the PR name follows the format `[GH-XXX] my subject`. Closes #2331
## What changes were proposed in this PR?
Unfortunately, Sedona doesn't have the exact same behavior for geopandas spatial index queries. Geopandas return indices, while Sedona returns actual geometries. It wouldn't be easy to modify Sedona to return the indices, nor does it make much sense to imo, since indexes would be very slow to maintain in a distributed environment.
- Additionally, this PR fixes an issue where we return [GeoData](https://github.com/apache/sedona/blob/e6e7c88bc8110bd10de36880ed8597313a3d8846/python/sedona/spark/utils/spatial_rdd_parser.py#L36) types from the RDD api for some of the functions. I don't see a need to keep the userData field, so I've converted so the user receives raw shapely geometries instead. This was already fixed in `query` in [this previous PR](https://github.com/apache/sedona/pull/2216), and this PR now applies it to the rest of the sindex functions.
## How was this patch tested?
Added new tests
## Did this PR include necessary documentation updates?
- Yes, I have updated the documentation.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2331] Geopandas: Document differences of sindex compared to gpd + sindex fixes [sedona]
Posted by "zhangfengcdt (via GitHub)" <gi...@apache.org>.
zhangfengcdt commented on PR #2332:
URL: https://github.com/apache/sedona/pull/2332#issuecomment-3259765394
> I think given we are building on Spark/Sedona, scalability and performance should be the top priority. We should stick with Option 2. The performance gain of the distributed approach far outweigh the cost of a documented breaking change, especially when the new workflow is arguably simpler for the user.
Yeah, see my comments above "I think given we are building on Spark/Sedona, scalability and performance should be the top priority. We should stick with Option 2. The performance gain of the distributed approach far outweigh the cost of a documented breaking change, especially when the new workflow is arguably simpler for the user."
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2331] Geopandas: Document differences of sindex compared to gpd + sindex fixes [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #2332:
URL: https://github.com/apache/sedona/pull/2332#discussion_r2325779811
##########
python/sedona/spark/geopandas/sindex.py:
##########
@@ -38,12 +38,23 @@ def __init__(self, geometry, index_type="strtree", column_name=None):
Parameters
----------
- geometry : np.array of Shapely geometries, PySparkDataFrame column, or PySparkDataFrame
+ geometry : np.array of Shapely geometries, GeoSeries, or PySparkDataFrame
index_type : str, default "strtree"
The type of spatial index to use.
column_name : str, optional
The column name to extract geometry from if `geometry` is a PySparkDataFrame.
+
+ Note: query methods (ie. query, nearest, intersection) have different behaviors depending on how the index is constructed.
+ When constructed from a np.array, the query methods return indices like original geopandas.
+ When constructed from a GeoSeries or PySparkDataFrame, the query methods return geometries.
Review Comment:
Here's the gist of the first point. Essentially, passing in a np.array will return indices like the original geopandas behavior (obviously, it will be slow). But passing in GeoSeries or PySparkDataFrame will return geometries. This is the best we can do in terms of compatibility.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2331] Geopandas: Document differences of sindex compared to gpd + sindex fixes [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on PR #2332:
URL: https://github.com/apache/sedona/pull/2332#issuecomment-3259377559
Sorry, I learned about this issue a little more recently and had it on my backlog of things to fix. I didn't realize how deep this went until I looked into it now. I think the question we need to decide here what should `GeoSeries.sindex` build by default? We have two options
- build from a np.array: non-distributed but returns indices like the original behavior
- build from a GeoSeries: distributed / fast, returns geometries (breaking difference)
Currently, it does the latter, which is my preference. WDYT @zhangfengcdt?
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2331] Geopandas: Document differences of sindex compared to gpd + sindex fixes [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu merged PR #2332:
URL: https://github.com/apache/sedona/pull/2332
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2331] Geopandas: Document differences of sindex compared to gpd + sindex fixes [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on PR #2332:
URL: https://github.com/apache/sedona/pull/2332#issuecomment-3259739514
@zhangfengcdt Are we good? Any thoughts on the direction?
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2331] Geopandas: Document differences of sindex compared to gpd + sindex fixes [sedona]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2332:
URL: https://github.com/apache/sedona/pull/2332#discussion_r2325822947
##########
python/sedona/spark/geopandas/sindex.py:
##########
@@ -170,10 +186,14 @@ def nearest(
Note: Unlike Geopandas, Sedona does not support geometry input of type np.array or GeoSeries.
+ Note: nearest() has different behaviors depending on how the index is constructed.
+ When constructed from a np.array, this method returns indices like original geopandas.
+ When constructed from a GeoSeries or PySparkDataFrame, this method returns geometries.
+
Returns
-------
list or tuple
- List of indices of nearest geometries, optionally with distances.
+ List of the nearest geometries, optionally with distances.
Review Comment:
Similar to the query method, this docstring states it returns geometries but should acknowledge that it returns indices when constructed from np.array, based on the behavioral notes above.
```suggestion
If the index was constructed from a np.array, returns a list of indices (or a tuple of (indices, distances) if return_distance is True).
If constructed from a GeoSeries or PySparkDataFrame, returns a list of geometries (or a tuple of (geometries, distances) if return_distance is True).
```
##########
python/tests/geopandas/test_sindex.py:
##########
@@ -63,6 +63,31 @@ def setup_method(self):
]
)
+ def test_construct_from_geoseries(self):
+ # Construct from a GeoSeries
+ gs = GeoSeries([Point(x, x) for x in range(5)])
+ sindex = SpatialIndex(gs)
+ result = sindex.query(Point(2, 2))
+ # SpatialIndex constructed from GeoSeries return geometries
+ assert result == [Point(2, 2)]
+
+ def test_construct_from_pyspark_dataframe(self):
+ # Construct from PySparkDataFrame
+ df = self.spark.createDataFrame(
+ [(Point(x, x),) for x in range(5)], ["geometry"]
+ )
+ sindex = SpatialIndex(df, column_name="geometry")
+ result = sindex.query(Point(2, 2))
+ assert result == [Point(2, 2)]
+
+ def test_construct_from_nparray(self):
+ # Construct from np.array
+ array = np.array([Point(x, x) for x in range(5)])
+ sindex = SpatialIndex(array)
+ result = sindex.query(Point(2, 2))
+ # Returns indices like original geopandas
+ assert result.tolist() == np.array([2])
Review Comment:
The assertion compares a list to a numpy array, which will always be False. Use `assert result.tolist() == [2]` or `assert result == np.array([2])` instead.
```suggestion
assert result.tolist() == [2]
```
##########
python/sedona/spark/geopandas/sindex.py:
##########
@@ -220,20 +243,28 @@ def nearest(
def intersection(self, bounds):
"""
- Find geometries that intersect the given bounding box.
+ Find geometries that intersect the given bounding box. Similar to the Geopandas version,
+ this is a compatibility wrapper for rtree.index.Index.intersection, use query instead.
Parameters
----------
bounds : tuple
Bounding box as (min_x, min_y, max_x, max_y).
+ Note: intersection() has different behaviors depending on how the index is constructed.
+ When constructed from a np.array, this method returns indices like original geopandas.
+ When constructed from a GeoSeries or PySparkDataFrame, this method returns geometries.
+
+ Note: Unlike Geopandas, Sedona does not support geometry input of type np.array or GeoSeries.
+ It is recommended to instead use GeoSeries.intersects directly.
+
Returns
-------
list
- List of indices of matching geometries.
+ List of matching geometries.
Review Comment:
The return description should be consistent with the behavioral notes and acknowledge that indices are returned when constructed from np.array.
```suggestion
List of matching indices (if constructed from a np.array) or list of matching geometries (if constructed from a GeoSeries or PySparkDataFrame).
```
##########
python/sedona/spark/geopandas/sindex.py:
##########
@@ -82,12 +93,17 @@ def query(self, geometry: BaseGeometry, predicate: str = None, sort: bool = Fals
sort : bool, optional, default False
Whether to sort the results.
+ Note: query() has different behaviors depending on how the index is constructed.
+ When constructed from a np.array, this method returns indices like original geopandas.
+ When constructed from a GeoSeries or PySparkDataFrame, this method returns geometries.
+
Note: Unlike Geopandas, Sedona does not support geometry input of type np.array or GeoSeries.
+ It is recommended to instead use GeoSeries.intersects directly.
Returns
-------
list
- List of indices of matching geometries.
+ List of matching geometries.
Review Comment:
The docstring states this returns 'List of matching geometries' but the note above indicates it returns indices when constructed from np.array. The return description should acknowledge both behaviors or be more generic.
```suggestion
If the index was constructed from a np.array, returns a list of indices of matching geometries.
If constructed from a GeoSeries or PySparkDataFrame, returns a list of matching geometries.
```
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2331] Geopandas: Document differences of sindex compared to gpd + sindex fixes [sedona]
Posted by "zhangfengcdt (via GitHub)" <gi...@apache.org>.
zhangfengcdt commented on PR #2332:
URL: https://github.com/apache/sedona/pull/2332#issuecomment-3259423557
> Sorry, I learned about this issue a little more recently and had it on my backlog of things to fix. I didn't realize how deep this went until I looked into it now. I think the question we need to decide here what should `GeoSeries.sindex` build by default? We have two options
>
> * build from a np.array: non-distributed but returns indices like the original behavior
> * build from a GeoSeries: distributed / fast, returns geometries (breaking difference)
>
> Currently, it does the latter, which is my preference. WDYT @zhangfengcdt?
I think given we are building on Spark/Sedona, scalability and performance should be the top priority. We should stick with Option 2. The performance gain of the distributed approach far outweigh the cost of a documented breaking change, especially when the new workflow is arguably simpler for the user.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2331] Geopandas: Document differences of sindex compared to gpd + sindex fixes [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #2332:
URL: https://github.com/apache/sedona/pull/2332#discussion_r2325840015
##########
python/tests/geopandas/test_sindex.py:
##########
@@ -63,6 +63,31 @@ def setup_method(self):
]
)
+ def test_construct_from_geoseries(self):
+ # Construct from a GeoSeries
+ gs = GeoSeries([Point(x, x) for x in range(5)])
+ sindex = SpatialIndex(gs)
+ result = sindex.query(Point(2, 2))
+ # SpatialIndex constructed from GeoSeries return geometries
+ assert result == [Point(2, 2)]
+
+ def test_construct_from_pyspark_dataframe(self):
+ # Construct from PySparkDataFrame
+ df = self.spark.createDataFrame(
+ [(Point(x, x),) for x in range(5)], ["geometry"]
+ )
+ sindex = SpatialIndex(df, column_name="geometry")
+ result = sindex.query(Point(2, 2))
+ assert result == [Point(2, 2)]
+
+ def test_construct_from_nparray(self):
+ # Construct from np.array
+ array = np.array([Point(x, x) for x in range(5)])
+ sindex = SpatialIndex(array)
+ result = sindex.query(Point(2, 2))
+ # Returns indices like original geopandas
+ assert result.tolist() == np.array([2])
Review Comment:
Seems to pass anyways, but I made the change anyways in my PR feedback commit
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2331] Geopandas: Document differences of sindex compared to gpd + sindex fixes [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #2332:
URL: https://github.com/apache/sedona/pull/2332#discussion_r2325841394
##########
python/sedona/spark/geopandas/sindex.py:
##########
@@ -82,12 +93,17 @@ def query(self, geometry: BaseGeometry, predicate: str = None, sort: bool = Fals
sort : bool, optional, default False
Whether to sort the results.
+ Note: query() has different behaviors depending on how the index is constructed.
+ When constructed from a np.array, this method returns indices like original geopandas.
+ When constructed from a GeoSeries or PySparkDataFrame, this method returns geometries.
+
Note: Unlike Geopandas, Sedona does not support geometry input of type np.array or GeoSeries.
+ It is recommended to instead use GeoSeries.intersects directly.
Returns
-------
list
- List of indices of matching geometries.
+ List of matching geometries.
Review Comment:
Good catches, I pushed a commit formatting it in a way I like better
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org