Copilot commented on code in PR #2216:
URL: https://github.com/apache/sedona/pull/2216#discussion_r2249631966
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -2917,9 +2907,11 @@ def to_parquet(self, path, **kwargs):
# # Utils
#
-----------------------------------------------------------------------------
- def _update_inplace(self, result: "GeoSeries"):
+ def _update_inplace(self, result: "GeoSeries", invalidate_sindex: bool =
True):
self.rename(result.name, inplace=True)
self._update_anchor(result._anchor)
+ if invalidate_sindex:
Review Comment:
[nitpick] The `invalidate_sindex` parameter should have a more descriptive
name like `preserve_sindex` with inverted logic, as this would better express
the intent when calling with `preserve_sindex=True` for operations like
`set_crs`.
```suggestion
def _update_inplace(self, result: "GeoSeries", preserve_sindex: bool =
False):
self.rename(result.name, inplace=True)
self._update_anchor(result._anchor)
if not preserve_sindex:
```
##########
python/tests/geopandas/test_geoseries.py:
##########
@@ -113,6 +113,48 @@ def test_to_spark_pandas(self):
ps_df = ps.Series(data, index=index)
assert_series_equal(result.to_pandas(), ps_df.to_pandas())
+ def test_sindex(self):
+ s = GeoSeries([Point(x, x) for x in range(5)])
+ assert not s.has_sindex
+
+ result = s.sindex.query(box(1, 1, 3, 3))
+ expected = [Point(1, 1), Point(2, 2), Point(3, 3)]
+ assert result == expected
+ assert s.has_sindex
+
+ result = s.sindex.query(box(1, 1, 3, 3), predicate="contains")
+ expected = [Point(1, 1), Point(2, 2), Point(3, 3)]
+ assert result == expected
+ assert s.has_sindex
+
+ # Check that it works with a GeoDataFrame
+ gdf = s.to_geoframe()
+ result = gdf.sindex.query(box(1, 1, 3, 3), predicate="contains")
+ assert result == expected
+
+ # This is challenging to support due to gdf.__setitem__ casting
GeoSeries into pspd.Series
Review Comment:
The comment should explain the workaround or reference a future improvement
plan, rather than just stating the challenge exists.
```suggestion
# This is currently not supported because gdf.__setitem__ casts
GeoSeries into pspd.Series,
# which prevents correct sindex tracking. See
https://github.com/apache/sedona/issues/1234
# for future plans to address this limitation. For now, we skip this
assertion.
```
--
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]
