-
Notifications
You must be signed in to change notification settings - Fork 742
[GH-2527] Implement concave_hull #2529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[GH-2527] Implement concave_hull #2529
Conversation
|
Hi @petern48 I'm encountering a test failure in our Sedona/GeoPandas parity suite for concave_hull(Passing in my local). The error is due to the vertex order of polygons returned by Sedona and GeoPandas being different, even though the polygons are geometrically identical. Our current comparison uses equals_exact(assert_geometry_almost_equal), which might be sensitive to vertex order:
https://github.com/apache/sedona/actions/runs/19687051539/job/56394868966?pr=2529 Would it be acceptable to change our comparison from equals_exact to equals for polygons, so that we check geometric equality rather than strict vertex order? Or is there a recommended workaround for this situation, or something I might be missing in our test setup? Thank you for your guidance! |
|
|
||
| mixed = [self.points[1], self.linestrings[1], self.polygons[1], None] | ||
| sgpd_result = GeoSeries(mixed).concave_hull() | ||
| gpd_result = gpd.GeoSeries(mixed).concave_hull() | ||
| self.check_sgpd_equals_gpd(sgpd_result, gpd_result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| mixed = [self.points[1], self.linestrings[1], self.polygons[1], None] | |
| sgpd_result = GeoSeries(mixed).concave_hull() | |
| gpd_result = gpd.GeoSeries(mixed).concave_hull() | |
| self.check_sgpd_equals_gpd(sgpd_result, gpd_result) |
I don't think this test is necessary. We're already testing all of those cases separately in the for geom in self.geoms above. The function is executed on each geometry separately, so whether they're mixed together or not doesn't matter.
| for geom in self.geoms: | ||
| sgpd_result = GeoSeries(geom).concave_hull() | ||
| gpd_result = gpd.GeoSeries(geom).concave_hull() | ||
| self.check_sgpd_equals_gpd(sgpd_result, gpd_result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should test multiple ratio values here, not just the default of 0.0. We could do something like: Do one third of the iterations w/ 0.0, then the next third w/ 0.5, and the last third w/ 1.0. (Note the range of valid values for this argument is [0, 1].
Then also test allow_holes below (as you already are doing).
| crs=3857, | ||
| ) | ||
|
|
||
| result = s.concave_hull() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have a test for allow_holes=True in this file too. But we should first get to the bottom of the errors.
The issue is not whether we're using Script to test using equals insteadAs you can see above, these geometries actually are different. Why this is happening, I'm not sure. It could either be a bug in Sedona's function or a difference in algorithms or edge case behaviors. If it's a significant bug, we may want to hold off. If it's a reasonable difference in behavior that's still correct, we can add a note in the docs and merge this anyway. Either way, we need to understand what's happening before merging. I encourage you to investigate, though you're not obligated to, of course, and can continue with something else instead. |


Did you read the Contributor Guide?
Is this PR related to a ticket?
[GH-XXX] my subject. Closes Geopandas: Implement concave_hull #2527What changes were proposed in this PR?
How was this patch tested?
Did this PR include necessary documentation updates?