-
Notifications
You must be signed in to change notification settings - Fork 132
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
h3shape_to_cells_experimental
takes string containment modes
#436
Changes from all commits
a579efb
fb2cf65
c5a5d65
c627dca
0a5eac0
81a96e2
022de27
49dbf59
4b805bf
fd13c95
69f48dd
68ccc36
a58ba05
287fa4c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
# This file is **symlinked** across the APIs to ensure they are exactly the same. | ||
from typing import Literal | ||
|
||
from ... import _cy | ||
from ..._h3shape import ( | ||
ContainmentMode, | ||
H3Shape, | ||
LatLngPoly, | ||
LatLngMultiPoly, | ||
|
@@ -468,6 +468,13 @@ def uncompact_cells(cells, res): | |
return _out_collection(hu) | ||
|
||
|
||
def polygon_to_cells(h3shape, res): | ||
""" | ||
Alias for ``h3shape_to_cells``. | ||
""" | ||
return h3shape_to_cells(h3shape, res) | ||
|
||
|
||
def h3shape_to_cells(h3shape, res): | ||
""" | ||
Return the collection of H3 cells at a given resolution whose center points | ||
|
@@ -519,25 +526,46 @@ def h3shape_to_cells(h3shape, res): | |
return _out_collection(mv) | ||
|
||
|
||
def polygon_to_cells(h3shape, res): | ||
def polygon_to_cells_experimental( | ||
h3shape: H3Shape, | ||
res: int, | ||
contain: Literal['center', 'full', 'overlap', 'bbox_overlap'] = 'center', | ||
): | ||
""" | ||
Alias for ``h3shape_to_cells``. | ||
Alias for ``h3shape_to_cells_experimental``. | ||
""" | ||
return h3shape_to_cells(h3shape, res) | ||
return h3shape_to_cells_experimental(h3shape, res, contain) | ||
|
||
|
||
def h3shape_to_cells_experimental(h3shape, res, flags=0): | ||
def h3shape_to_cells_experimental( | ||
h3shape: H3Shape, | ||
res: int, | ||
contain: Literal['center', 'full', 'overlap', 'bbox_overlap'] = 'center', | ||
): | ||
""" | ||
Return the collection of H3 cells at a given resolution whose center points | ||
are contained within an ``LatLngPoly`` or ``LatLngMultiPoly``. | ||
Experimental function similar to ``h3shape_to_cells``, but with support for | ||
multiple cell containment modes. | ||
|
||
Using ``contain='center'`` should give identical behavior as | ||
``h3shape_to_cells``. | ||
|
||
Note that this function is **experimental** and has no API stability gaurantees | ||
across versions, so it may change in the future. | ||
|
||
|
||
Parameters | ||
---------- | ||
h3shape : ``H3Shape`` | ||
res : int | ||
Resolution of the output cells | ||
flags : ``ContainmentMode``, int, or string | ||
Containment mode flags | ||
contain : {'center', 'full', 'overlap', 'bbox_overlap'}, optional | ||
Specifies the containment condition. | ||
- 'center': Cell center is contained in shape | ||
- 'full': Cell is fully contained in shape | ||
- 'overlap': Cell is partially contained in shape | ||
- 'bbox_overlap': Cell bounding box is partially contained in shape | ||
|
||
Default is 'center'. | ||
|
||
Returns | ||
------- | ||
|
@@ -550,7 +578,7 @@ def h3shape_to_cells_experimental(h3shape, res, flags=0): | |
... [(37.68, -122.54), (37.68, -122.34), (37.82, -122.34), | ||
... (37.82, -122.54)], | ||
... ) | ||
>>> h3.h3shape_to_cells_experimental(poly, 6, h3.ContainmentMode.containment_center) | ||
>>> h3.h3shape_to_cells_experimental(poly, 6, 'center') | ||
['862830807ffffff', | ||
'862830827ffffff', | ||
'86283082fffffff', | ||
|
@@ -564,30 +592,27 @@ def h3shape_to_cells_experimental(h3shape, res, flags=0): | |
There is currently no guaranteed order of the output cells. | ||
""" | ||
|
||
if isinstance(flags, str): | ||
try: | ||
flags = ContainmentMode[flags] | ||
except KeyError as e: | ||
raise ValueError('Unrecognized flags: ' + flags) from e | ||
if isinstance(flags, ContainmentMode): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally I would prefer to keep the enum option in addition to the stringly-typed form. Would typing pick up the available string options and offer them as suggestions? I don't see them in type annotations right now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think typing in the future is the more natural way to handle this. We don't currently have types, but they could be added. We don't use them in the area/length functions, for example, to specify the output units. Thus, the current form is a smaller change that's more consistent with the existing library design, and doesn't preclude adding typing or enums in the future.
Yes, that's my understanding, if we were to add the type annotations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And actually, if you're interested in adding support for IDE tooling like autocomplete/suggestions, I think we should do that in a separate issue or PR. That's outside the scope of this PR (or #432), as it affects the library as a whole. And I'd definitely welcome that discussion, as I think it is needed! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You know what? The function is experimental, so let's just try it and see how it works. I do still think we should separately take a look at typing in the library holistically. |
||
flags = int(flags) | ||
if not isinstance(flags, int): | ||
raise ValueError( | ||
'Flags should be ContainmentMode, str, or int, but got: ' + str(type(flags)) | ||
) | ||
contain_modes = { | ||
'center': 0, | ||
'full': 1, | ||
'overlap': 2, | ||
'bbox_overlap': 3, | ||
} | ||
|
||
flag = contain_modes[contain] | ||
|
||
# todo: not sure if i want this dispatch logic here. maybe in the objects? | ||
if isinstance(h3shape, LatLngPoly): | ||
poly = h3shape | ||
mv = _cy.polygon_to_cells_experimental( | ||
poly.outer, | ||
res=res, | ||
holes=poly.holes, | ||
flags=flags | ||
res = res, | ||
holes = poly.holes, | ||
flag = flag, | ||
) | ||
elif isinstance(h3shape, LatLngMultiPoly): | ||
mpoly = h3shape | ||
mv = _cy.polygons_to_cells_experimental(mpoly.polys, res=res, flags=flags) | ||
mv = _cy.polygons_to_cells_experimental(mpoly.polys, res=res, flag=flag) | ||
elif isinstance(h3shape, H3Shape): | ||
raise ValueError('Unrecognized H3Shape: ' + str(h3shape)) | ||
else: | ||
|
@@ -596,13 +621,6 @@ def h3shape_to_cells_experimental(h3shape, res, flags=0): | |
return _out_collection(mv) | ||
|
||
|
||
def polygon_to_cells_experimental(h3shape, res, flags=0): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the name of the function in the C library and on the docs site. Why is the alias being removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we discussed this previously, and the preferred function name in As this function is experimental, and we already have the pointer, I think adding an additional alias is redundant. Also, we'll get back to the status quo when we merge the experimental function into the primary function. |
||
""" | ||
Alias for ``h3shape_to_cells_experimental``. | ||
""" | ||
return h3shape_to_cells_experimental(h3shape, res, flags=flags) | ||
|
||
|
||
def cells_to_h3shape(cells, *, tight=True): | ||
""" | ||
Return an ``H3Shape`` describing the area covered by a collection of H3 cells. | ||
|
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.
This does not seem like an idiomatic change to me. While containment modes cannot be combined, I think the intention was to expand flags to cover other cases such as spherical vs Cartesian.
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.
The Cython function are internal and not part of the externally supported API, so we're free to change this in the future without officially breaking anything.