Skip to content
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

Added Bindings for Vertex-Related Functions #388

Merged
merged 27 commits into from
Aug 27, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ avoid adding features or APIs which do not map onto the

## Unreleased

- None
## [4.0.0b6] - 2024-08-23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just put this under ## Unreleased for now, since we'll publish as a separate step.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK!


- Added bindings for `cellToVertex`, `cellToVertexes`, `vertexToLatLng`, and `isValidVertex` (#323)

## [4.0.0b5] - 2024-05-19

Expand Down
4 changes: 4 additions & 0 deletions src/h3/_cy/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@
cell_to_boundary,
directed_edge_to_boundary,
great_circle_distance,
cell_to_vertex,
cell_to_vertexes,
vertex_to_latlng,
is_valid_vertex,
)

from .to_multipoly import (
Expand Down
5 changes: 5 additions & 0 deletions src/h3/_cy/h3lib.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ cdef extern from 'h3api.h':
int isPentagon(H3int h) nogil
int isResClassIII(H3int h) nogil
int isValidDirectedEdge(H3int edge) nogil
int isValidVertex(H3int v) nogil

double degsToRads(double degrees) nogil
double radsToDegs(double radians) nogil
Expand All @@ -80,6 +81,10 @@ cdef extern from 'h3api.h':
H3Error cellToLatLng(H3int h, LatLng *) nogil
H3Error gridDistance(H3int h1, H3int h2, int64_t *distance) nogil

H3Error cellToVertex(H3int cell, int vertexNum, H3int *out) nogil
H3Error cellToVertexes(H3int cell, H3int *vertexes) nogil
H3Error vertexToLatLng(H3int vertex, LatLng *coord) nogil

H3Error maxGridDiskSize(int k, int64_t *out) nogil # num/out/N?
H3Error gridDisk(H3int h, int k, H3int *out) nogil

Expand Down
6 changes: 5 additions & 1 deletion src/h3/_cy/latlng.pxd
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
from .h3lib cimport H3int
from .h3lib cimport bool, H3int

cpdef H3int latlng_to_cell(double lat, double lng, int res) except 1
cpdef (double, double) cell_to_latlng(H3int h) except *
cpdef double great_circle_distance(
double lat1, double lng1,
double lat2, double lng2, unit=*) except -1
cpdef H3int cell_to_vertex(H3int h, int vertex_num) except 1
cpdef H3int[:] cell_to_vertexes(H3int h)
cpdef (double, double) vertex_to_latlng(H3int v) except *
cpdef bool is_valid_vertex(H3int v)
46 changes: 45 additions & 1 deletion src/h3/_cy/latlng.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ from h3lib cimport bool, H3int
from .util cimport (
check_cell,
check_edge,
check_vertex,
check_res,
deg2coord,
coord2deg,
coord2deg
)

from .error_system cimport check_for_error
Expand Down Expand Up @@ -245,3 +246,46 @@ cpdef double great_circle_distance(
raise ValueError('Unknown unit: {}'.format(unit))

return d

# todo: one might want to move the following to a separate vertex.pyx file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm supportive of moving this to a separate vertex.pyx file in this PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but I thought that would be a larger change of structure... I wrote this comment in case there would be more vertex-related functions in the future. IMHO the current structure is good enough, but I can move it to a separate file as well if it's very beneficial :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate you asking! I think your suggestion to move the vertex code to its own file is a good one. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'd do so.


cpdef H3int cell_to_vertex(H3int h, int vertex_num) except 1:
cdef:
H3int out

check_cell(h)

check_for_error(
h3lib.cellToVertex(h, vertex_num, &out)
)

return out

cpdef H3int[:] cell_to_vertexes(H3int h):
cdef:
H3int out

check_cell(h)

hmm = H3MemoryManager(6)
check_for_error(
h3lib.cellToVertexes(h, hmm.ptr)
)
mv = hmm.to_mv()

return mv

cpdef (double, double) vertex_to_latlng(H3int v) except *:
cdef:
h3lib.LatLng c

check_vertex(v)

check_for_error(
h3lib.vertexToLatLng(v, &c)
)

return coord2deg(c)

cpdef bool is_valid_vertex(H3int v):
return h3lib.isValidVertex(v) == 1
1 change: 1 addition & 0 deletions src/h3/_cy/util.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ cpdef H3str int_to_str(H3int x)
cdef check_cell(H3int h)
cdef check_edge(H3int e)
cdef check_res(int res)
cdef check_vertex(H3int v)
cdef check_distance(int k)
7 changes: 6 additions & 1 deletion src/h3/_cy/util.pyx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from .h3lib cimport H3int, H3str, isValidCell, isValidDirectedEdge
from .h3lib cimport H3int, H3str, isValidCell, isValidDirectedEdge, isValidVertex

cimport h3lib

Expand All @@ -7,6 +7,7 @@ from .error_system import (
H3DomainError,
H3DirEdgeInvalidError,
H3CellInvalidError,
H3VertexInvalidError
)

cdef h3lib.LatLng deg2coord(double lat, double lng) nogil:
Expand Down Expand Up @@ -73,6 +74,10 @@ cdef check_edge(H3int e):
if isValidDirectedEdge(e) == 0:
raise H3DirEdgeInvalidError('Integer is not a valid H3 edge: {}'.format(hex(e)))

cdef check_vertex(H3int v):
if isValidVertex(v) == 0:
raise H3VertexInvalidError('Integer is not a valid H3 vertex: {}'.format(hex(v)))

cdef check_res(int res):
if (res < 0) or (res > 15):
raise H3ResDomainError(res)
Expand Down
2 changes: 1 addition & 1 deletion src/h3/_version.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
__version__ = '4.0.0b5'
__version__ = '4.0.0b6'
__description__ = 'Hierarchical hexagonal geospatial indexing system'
__url__ = 'https://github.com/uber/h3-py'
__license__ = 'Apache 2.0 License'
Expand Down
67 changes: 67 additions & 0 deletions src/h3/api/basic_int/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -996,3 +996,70 @@ def great_circle_distance(latlng1, latlng2, unit='km'):
lat2, lng2,
unit = unit
)


def cell_to_vertex(h, vertex_num):
"""
Return a (specified) vertex of an H3 cell.

Parameters
----------
h : H3Cell
vertex_num : int
Vertex number (0-5)

Returns
-------
The vertex
"""
return _cy.cell_to_vertex(_in_scalar(h), vertex_num)


def cell_to_vertexes(h):
"""
Return a list of vertices of an H3 cell. The list will always be of length 6.
If a pentagon is entered, the last element will be 0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Python world, I'd rather have us dynamically size the array: a hex should return an array of length 6, and a pentagon should return an array of length 5.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I fixed array length to 6 only to align with the official docs... I'll change this.


Parameters
----------
h : H3Cell

Returns
-------
A list of vertices
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think we should have used "vertices" in H3, we decided to go with "vertexes". As a result, I think we should try to be consistent in the code and docs and only use "vertexes".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK! :-)

"""
mv = _cy.cell_to_vertexes(_in_scalar(h))
arr = [str_to_int(a) for a in _out_collection(mv)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making adjustments, but this will still return ints for each API. We want the output to depend on the API:

  • list of int for h3.api.basic_int
  • list of str for h3.api.basic_str
  • numpy.array for h3.api.numpy_int
  • memoryview of int for h3.api.memview_int

To do that, the last operation should be _out_collcation(mv). The _remove_zeros() function should help with this: https://github.com/uber/h3-py/blob/master/src/h3/_cy/memory.pyx#L113

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line still needed? It looks as if you can return _out_collection(mv)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have rewritten in my newest commit (yesterday)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Github is still showing this comment as up-to-date, could you double check your commit has been pushed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, thank you! Fixed.

return arr if len(arr) == 6 else arr + [0]


def vertex_to_latlng(v):
"""
Return latitude and longitude of a vertex.

Returns
-------
lat : float
Latitude
lng : float
Longitude
"""
if isinstance(v, str):
v = str_to_int(v)
return _cy.vertex_to_latlng(v)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to use _in_scalar() so that this function will work as expected for the other APIs (basic_int, basic_str, etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK!



def is_valid_vertex(v):
"""
Validates an H3 vertex.

Returns
-------
bool
"""
try:
if isinstance(v, str):
v = str_to_int(v)
return _cy.is_valid_vertex(v)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use _in_scalar() here as well. See how we do it in is_valid_cell, is_valid_directed_edge, and is_pentagon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK!

except (ValueError, TypeError):
return False
59 changes: 59 additions & 0 deletions tests/test_h3.py
Original file line number Diff line number Diff line change
Expand Up @@ -387,3 +387,62 @@ def test_grid_path_cells():

with pytest.raises(h3.H3ResMismatchError):
h3.grid_path_cells(h1, '8001fffffffffff')


def test_cell_to_vertex():
# pentagon
assert h3.cell_to_vertex('814c3ffffffffff', 0) == 2311688013026951167
assert h3.cell_to_vertex('814c3ffffffffff', 1) == 2383745607064879103
assert h3.cell_to_vertex('814c3ffffffffff', 2) == 2455803201102807039
assert h3.cell_to_vertex('814c3ffffffffff', 3) == 2527860795140734975
assert h3.cell_to_vertex('814c3ffffffffff', 4) == 2599918389178662911
try:
h3.cell_to_vertex('814c3ffffffffff', 5)
except h3._cy.error_system.H3DomainError:
pass
else:
assert False

# hexagon
assert h3.cell_to_vertex('814d7ffffffffff', 0) == 2311710003259506687
assert h3.cell_to_vertex('814d7ffffffffff', 1) == 2455495337847029759
assert h3.cell_to_vertex('814d7ffffffffff', 2) == 2383437743809101823
assert h3.cell_to_vertex('814d7ffffffffff', 3) == 2599918389178662911
assert h3.cell_to_vertex('814d7ffffffffff', 4) == 2527860795140734975
assert h3.cell_to_vertex('814d7ffffffffff', 5) == 2599931583318196223


def test_cell_to_vertexes():
# pentagon
assert h3.cell_to_vertexes('814c3ffffffffff') == [
2311688013026951167,
2383745607064879103,
2455803201102807039,
2527860795140734975,
2599918389178662911,
0
]

# hexagon
assert h3.cell_to_vertexes('814d7ffffffffff') == [
2311710003259506687,
2455495337847029759,
2383437743809101823,
2599918389178662911,
2527860795140734975,
2599931583318196223,
]


def test_vertex_to_latlng():
latlng = h3.vertex_to_latlng('2114c3ffffffffff')
assert latlng == approx((24.945215618732814, -70.33904370008679))
latlng = h3.vertex_to_latlng(2455495337847029759)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't expect any single API to be able to handle both str and int inputs. The current design expects them to select an API explicitly, like h3.api.basic_int or h3.api.basic_str (the default).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I'll just leave the string tests, then.

assert latlng == approx((21.8146389946083, -57.23011196400803))


def test_is_valid_vertex():
assert h3.is_valid_vertex('2114c3ffffffffff')
assert h3.is_valid_vertex(2455495337847029759)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should expect this to fail for the (default) string API because a string is expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK!

assert not h3.is_valid_vertex('foobar')
assert not h3.is_valid_vertex(42)
Loading