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/07/23 17:42:18 UTC
[PR] [GH-2097] Geopandas: Clean up tests, reduce test time, add LinearRing tests [sedona]
petern48 opened a new pull request, #2145:
URL: https://github.com/apache/sedona/pull/2145
## 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 #2097
## What changes were proposed in this PR?
Improvements to geopandas testing:
-
## How was this patch tested?
Added new LinearRing tests.
## Did this PR include necessary documentation updates?
- No, this PR does not affect any public API so no need to change 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-2097] Geopandas: Clean up tests, reduce test time, add LinearRing tests [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #2145:
URL: https://github.com/apache/sedona/pull/2145#discussion_r2226273069
##########
python/tests/geopandas/test_match_geopandas_series.py:
##########
@@ -101,35 +106,35 @@ def setup_method(self):
)
]
- # (sql_table_name, geom)
self.geoms = [
- ("points", self.points),
- ("multipoints", self.multipoints),
- ("linestrings", self.linestrings),
- ("multilinestrings", self.multilinestrings),
- ("polygons", self.polygons),
- ("multipolygons", self.multipolygons),
- ("geomcollection", self.geomcollection),
+ self.points,
+ self.multipoints,
+ self.linestrings,
+ self.linearrings,
+ self.multilinestrings,
+ self.polygons,
+ self.multipolygons,
+ self.geomcollection,
]
- # create the tables in sedona spark
- for i, (table_name, geoms) in enumerate(self.geoms):
- wkt_string = [g.wkt for g in geoms]
- pd_df = pd.DataFrame({"id": i, "geometry": wkt_string})
- spark_df = self.spark.createDataFrame(pd_df)
- spark_df.createOrReplaceTempView(table_name)
+ self.pairs = [
+ (self.points, self.multipolygons),
+ (self.geomcollection, self.polygons),
+ (self.linestrings, self.multipoints),
+ (self.linearrings, self.multilinestrings),
+ ]
Review Comment:
The main change is this to loop through self.pairs instead of doing the double nested loop below. I think self.pairs here is exhaustive enough. This cuts test execution down by a lot for all of the predicate tests (e.g interesection)
```
for geom in self.geoms:
for geom2 in self.geoms:
```
--
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-2097] Geopandas: Clean up tests, reduce test time, add LinearRing tests [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on PR #2145:
URL: https://github.com/apache/sedona/pull/2145#issuecomment-3109676606
@jiayuasu I'd like to merge this before any of my other PRs.
--
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-2097] Geopandas: Clean up tests, reduce test time, add LinearRing tests [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on PR #2145:
URL: https://github.com/apache/sedona/pull/2145#issuecomment-3110147882
Addressed copilot feedback and all passing. Same with https://github.com/apache/sedona/pull/2111 after I merged this branch into that one. Should be a clean merge.
--
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-2097] Geopandas: Clean up tests, reduce test time, add LinearRing tests [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #2145:
URL: https://github.com/apache/sedona/pull/2145#discussion_r2226379655
##########
python/tests/geopandas/test_match_geopandas_series.py:
##########
@@ -346,17 +351,18 @@ def test_explode(self):
pass
def test_to_crs(self):
- for _, geom in self.geoms:
+ for geom in self.geoms[:3]:
sgpd_result = GeoSeries(geom, crs=4326)
gpd_result = gpd.GeoSeries(geom, crs=4326)
self.check_sgpd_equals_gpd(sgpd_result, gpd_result)
Review Comment:
Originally, it was just an arbitrary number to split it down the line, but then I realized I refactored the test incorrectly, so I've now fixed it.
--
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-2097] Geopandas: Clean up tests, reduce test time, add LinearRing tests [sedona]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2145:
URL: https://github.com/apache/sedona/pull/2145#discussion_r2226355430
##########
python/tests/geopandas/test_match_geopandas_series.py:
##########
@@ -346,17 +351,18 @@ def test_explode(self):
pass
def test_to_crs(self):
- for _, geom in self.geoms:
+ for geom in self.geoms[:3]:
sgpd_result = GeoSeries(geom, crs=4326)
gpd_result = gpd.GeoSeries(geom, crs=4326)
self.check_sgpd_equals_gpd(sgpd_result, gpd_result)
Review Comment:
The magic number '3' is used to slice geoms without explanation. Consider adding a comment explaining why only geoms[3:] are processed differently in this CRS transformation test, or use a more descriptive approach like filtering by geometry type.
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -4285,6 +4285,9 @@ def to_json(
*kwargs* that will be passed to json.dumps().
+ Note: Unlike geopandas, Sedona's implementation will specify replace 'LinearRing'
Review Comment:
The phrase 'will specify replace' is grammatically incorrect. It should be either 'will replace' or 'will specify to replace'.
```suggestion
Note: Unlike geopandas, Sedona's implementation will replace 'LinearRing'
```
##########
python/tests/geopandas/test_match_geopandas_series.py:
##########
@@ -346,17 +351,18 @@ def test_explode(self):
pass
def test_to_crs(self):
- for _, geom in self.geoms:
+ for geom in self.geoms[:3]:
sgpd_result = GeoSeries(geom, crs=4326)
gpd_result = gpd.GeoSeries(geom, crs=4326)
self.check_sgpd_equals_gpd(sgpd_result, gpd_result)
+ for geom in self.geoms[3:]:
Review Comment:
The magic number '3' is used to slice geoms without explanation. Consider adding a comment explaining why only the first 3 geometry types are processed in this CRS transformation test, or use a more descriptive approach like filtering by geometry type.
```suggestion
# Process only Point, LineString, and Polygon geometries for CRS transformation
filtered_geoms = [geom for geom in self.geoms if isinstance(geom, (Point, LineString, Polygon))]
for geom in filtered_geoms:
sgpd_result = GeoSeries(geom, crs=4326)
gpd_result = gpd.GeoSeries(geom, crs=4326)
self.check_sgpd_equals_gpd(sgpd_result, gpd_result)
for geom in filtered_geoms:
```
--
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-2097] Geopandas: Clean up tests, reduce test time, add LinearRing tests [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #2145:
URL: https://github.com/apache/sedona/pull/2145#discussion_r2226273069
##########
python/tests/geopandas/test_match_geopandas_series.py:
##########
@@ -101,35 +106,35 @@ def setup_method(self):
)
]
- # (sql_table_name, geom)
self.geoms = [
- ("points", self.points),
- ("multipoints", self.multipoints),
- ("linestrings", self.linestrings),
- ("multilinestrings", self.multilinestrings),
- ("polygons", self.polygons),
- ("multipolygons", self.multipolygons),
- ("geomcollection", self.geomcollection),
+ self.points,
+ self.multipoints,
+ self.linestrings,
+ self.linearrings,
+ self.multilinestrings,
+ self.polygons,
+ self.multipolygons,
+ self.geomcollection,
]
- # create the tables in sedona spark
- for i, (table_name, geoms) in enumerate(self.geoms):
- wkt_string = [g.wkt for g in geoms]
- pd_df = pd.DataFrame({"id": i, "geometry": wkt_string})
- spark_df = self.spark.createDataFrame(pd_df)
- spark_df.createOrReplaceTempView(table_name)
+ self.pairs = [
+ (self.points, self.multipolygons),
+ (self.geomcollection, self.polygons),
+ (self.linestrings, self.multipoints),
+ (self.linearrings, self.multilinestrings),
+ ]
Review Comment:
The main change is to loop through self.pairs instead of doing the double nested loop below. I think self.pairs here is exhaustive enough. This cuts test execution down by a lot for all of the predicate tests (e.g interesection)
```
for geom in self.geoms:
for geom2 in self.geoms:
```
--
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-2097] Geopandas: Clean up tests, reduce test time, add LinearRing tests [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu merged PR #2145:
URL: https://github.com/apache/sedona/pull/2145
--
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