[GH-3017] Implement rotate#3018
Conversation
|
Hey @petern48, I have tried to follow along with the instructions on ticket 2230 and PR example 2231 but if I missed anything on this please let me know! Thank you for putting those instructions together they were very helpful! |
petern48
left a comment
petern48
left a comment
There was a problem hiding this comment.
Thanks for working on this. I've done an initial scan and left some feedback
| Examples | ||
| -------- | ||
| >>> from shapely.geometry import Point, LineString, Polygon | ||
| >>> s = geopandas.GeoSeries( |
There was a problem hiding this comment.
| >>> s = geopandas.GeoSeries( | |
| >>> from sedona.spark.geopandas import GeoSeries | |
| >>> s = GeoSeries( |
we should update the import for sedona instead of geopandas
| sgpd_result = GeoSeries(geom).rotate(45, origin=(0, 0)) | ||
| gpd_result = gpd.GeoSeries(geom).rotate(45, origin=(0, 0)) |
There was a problem hiding this comment.
can we update these to an origin other than (0, 0), just to make sure it works? maybe something like (1, 2).
can we also add add a case for testing "centroid" and a Point() object as input for origin field to make sure they work?
| gpd_s = gpd.GeoSeries(geoms) | ||
|
|
||
| # Test default (degrees, origin='center') | ||
| self.check_sgpd_equals_gpd(s.rotate(90), gpd_s.rotate(90)) |
There was a problem hiding this comment.
test_geoseries.py should be testing the hard-coded expected outputs instead of comparing it directly to the result of what geopandas returns (that's what test_match_geopandas_series.py` is for. Could you update all of these examples to compare to the hard-coded expected output geometries?
|
@petern48 thank you for reviewing this and for the feedback on it I really appreciate it! I've gone back through the tests and have tried to update them using the suggestions you have provided - if I missed anything on them or need to be more explicit in some of the expected results just let me know. Thanks again! |
jiayuasu
left a comment
jiayuasu
left a comment
There was a problem hiding this comment.
Review notes on the rotate implementation and coverage.
| def test_rotate(self, angle, origin_kwargs): | ||
| for geom in self.geoms: | ||
| # Sedona converts empty geometries to None, skip them | ||
| if not gpd.GeoSeries(geom).is_empty.any(): |
There was a problem hiding this comment.
This condition skips every fixture group because each list in self.geoms starts with at least one empty geometry, so the parametrized cases pass without running any rotate comparisons. Could we filter empty geometries within each group, or build non-empty rotate fixtures, and replace origin="point" with an actual Point origin or an explicit invalid-origin case?
There was a problem hiding this comment.
ahhh, thank you for reviewing this and catching that! I'll get that updated and fixed!
| raise ValueError( | ||
| f"origin must be 'center', 'centroid', a Point, or (x, y) tuple, got {origin!r}" | ||
| ) | ||
| elif isinstance(origin, tuple) and len(origin) == 2: |
There was a problem hiding this comment.
GeoPandas/Shapely also accepts a two-element list for origin, for example origin=[0, 0], but this branch only accepts tuples. Since this wrapper is matching GeoPandas semantics, can we accept any two-element coordinate sequence here and add a list-origin regression test?
|
@jiayuasu I looked around at the other tests and noticed that |
3 participants
Did you read the Contributor Guide?
Is this PR related to a ticket?
[GH-3017] Implement rotate. Closes Geopandas: Implementrotate#3017What changes were proposed in this PR?
Implement rotate
How was this patch tested?
Added tests, verified success of all tests with:
117 passed, 44108 warnings in 140.69s (0:02:20)
Verified formatting with:
Did this PR include necessary documentation updates?