Introduce LatLngPoly and LatLngMultiPoly #364
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #364 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 319 319
=========================================
Hits 319 319 ☔ View full report in Codecov by Sentry. |
|
Gotta drops builds like #363 |
src/h3/api/basic_int/__init__.py
Outdated
| @@ -395,14 +395,14 @@ def uncompact_cells(cells, res): | |||
| return _out_collection(hu) | |||
|
|
|||
|
|
|||
| def h3shape_to_cells(h3shape, res): | |||
| def h3shape_to_cells(shape, res): | |||
There was a problem hiding this comment.
FWIW - I don't want to second-guess this, but I'm not sure how much this approach offers over just offering polygon_to_cells and making the caller break multi-polygons into polygons. The caller still has to handle this, by choosing LatLngPoly or LatLngMultiPoly when they convert their input data, but in this context you have to handle the both shapes in separate clauses and lose the clarity of sticking to the core library API. I'm not sure that trade-off is worthwhile for a slight ergonomic improvement when prepping input.
That said, I'm sure you've thought through this, so I'm okay with this API if you think it's worthwhile.
There was a problem hiding this comment.
I was imagining this being helpful in the API because everything is symmetric and there are no special cases. We have three classes of objects: "geo objects", H3Shapes, and cells. For any two types, there's a function that translates between them, and every function has an obvious inverse. To me, that's a nice complete set of functionality, and there are no gotchas like "oh, right... for that case, I need to give it a list of polygons".
And I don't think it is true that the caller has to handle it by choosing either LatLngPoly or LatLngMultiPoly. I think the typical use case is actually that they call geo_to_h3shape() on a GeoPandas column (which are often a mix of Shapely Polygons and MultiPolygons). So think this aligns more with what Python folks expect from working with geopandas.
Also, we have a MultiPolygon class in the core API (its just kind of hard to use), so I think this actually still matches the C code.
There was a problem hiding this comment.
But I do generally take your point. I agree we should try to align with the C library as close as possible unless there's a good reason otherwise.
Following from #332.
I'm leaving
H3Shapeinstead of changing to justShapebecause that seems too generic. And I think functions likegeo_to_shapewould be more confusing thangeo_to_h3shape. Thoughts?I'm also open to suggestions for a better name than
H3Shape.