H3Shape subclasses accept __geo_interface__ arguments #322
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## poly #322 +/- ##
===========================================
- Coverage 100.00% 90.85% -9.15%
===========================================
Files 18 18
Lines 389 492 +103
===========================================
+ Hits 389 447 +58
- Misses 0 45 +45
☔ View full report in Codecov by Sentry. |
| if len(holes) != 0: | ||
| raise ValueError("When copying from GeoJSON, holes cannot be specified") | ||
| self.outer = to_copy.polys[0].outer | ||
| self.holes = to_copy.polys[0].holes |
There was a problem hiding this comment.
Here and above - just use self.holes = tuple() for clarity?
There was a problem hiding this comment.
Don't think I understand what line(s) are being referenced?
| ll2 = [ | ||
| _swap_latlng(ll1) | ||
| for ll1 in ll2 | ||
| ] |
There was a problem hiding this comment.
Do you need to unclose the ring (i.e. delete the last coord if it duplicates the first)? Not really required for H3 code, but _polygon_to_LL2 and _LL2_to_polygon won't be fully symmetrical if the polygon input isn't closed. I guess this would be better enforced in the H3Poly constructor than here.
There was a problem hiding this comment.
Agreed that it would be nice to make these symmetric. I'd do that in the shape_to_geo function as described here: #322 (comment)
| t = gj_dict['type'] | ||
| coord = gj_dict['coordinates'] | ||
| # functions below should be inverses of each other | ||
| def _LL3_to_geojson_dict(ll3): |
There was a problem hiding this comment.
Just a nit, but would it be more legible to use loops and loop instead of LL3 and LL2?
There was a problem hiding this comment.
I do like this LL3/LL2 thing as it it seems clear what level of nesting is involved. That said it is not standard terminology.
There was a problem hiding this comment.
Is there standard terminology we could switch to?
There was a problem hiding this comment.
Perhaps loop/loops or ring/rings or something like that?
| @@ -0,0 +1,8 @@ | |||
| class MockGeoInterface: | |||
|
So However, |
src/h3/_polygon.py
Outdated
| @@ -220,8 +270,3 @@ def from_geo_interface(x): | |||
| ll3 = _geojson_dict_to_LL3(x) | |||
| mpoly = _LL3_to_mpoly(ll3) | |||
| return mpoly | |||
There was a problem hiding this comment.
Looks like we can remove check_geo_interface and from_geo_interface since they aren't used elsewhere.
|
@isaacbrodsky, this looks great and ready to land into the I have a few thoughts, but I'm happy to land this for now and follow up with changes on the I'm thinking about terminology and what would make the function behavior the most clear. It seems to me that we have four domains that we are translating between:
Right now, the code uses "shape" to refer to both 3 and 4. We also do translations between the domains in three places: For identifying the various domains, I'm considering the names (but very open to other suggestions):
If you have a nested list of lat/lng pairs, you just use the constructors for Those names lead us to the following functions/interface:
That being said, I imagine that most users will simply use Again, @isaacbrodsky, I'm happy to land this PR as is, and follow up with these right after. |
Drop 2.7 tests and avoid Cython warning
This builds on #301 to support passing objects with
__geo_interface__, such as Shapely geometries, into H3.I didn't add tests beyond what was already there and what was in the example notebook. Presumably we would want to add tests for this functionality specifically, based on the notebook.
I also assume we would remove
poly/util.pyas it appears to only exist for development purposes.