Added Bindings for Vertex-Related Functions #388

Shiran-Yuan · 2024-08-24T03:12:34Z

Added bindings and associated tests for the functions cellToVertex, cellToVertexes, vertexToLatLng, isValidVertex, and check_vertex per #323, ending the h3-py repo's longstanding disagreement with official documentation.

Vertices are returned as integers by default in cellToVertex and cellToVertexes, but inputs to vertexToLatLng and isValidVertex can be either integers or strings (the two forms are equivalent via int_to_str and str_to_int).

Affected files:

src/h3/_cy/__init__.py: added declarations
latlng.pxd and latlng.pyx: wrote the functions
util.pxd and util.pyx: added check_vertex
test_h3.py: added tests
src/h3/api/basic_int/__init__.py (and its aliases): added api access
h3lib.pxd: added declarations for the original functions in h3lib
CHANGELOG.md and _version.py: suggested version number 4.0.0b6

CLAassistant · 2024-08-24T03:12:41Z

All committers have signed the CLA.

ajfriend · 2024-08-24T22:07:19Z

This is great. Thanks for the PR! I'll take a look.

ajfriend · 2024-08-24T22:10:41Z

CHANGELOG.md

@@ -16,7 +16,9 @@ avoid adding features or APIs which do not map onto the

 ## Unreleased

- None
+## [4.0.0b6] - 2024-08-23


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

ajfriend · 2024-08-24T22:11:54Z

src/h3/_cy/latlng.pyx

@@ -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


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

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 :-)

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

Ok I'd do so.

ajfriend · 2024-08-24T22:15:47Z

src/h3/api/basic_int/__init__.py

+    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.


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.

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

ajfriend · 2024-08-24T22:17:11Z

src/h3/api/basic_int/__init__.py

+
+    Returns
+    -------
+    A list of vertices


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".

ajfriend · 2024-08-24T22:23:23Z

src/h3/api/basic_int/__init__.py

+    if isinstance(v, str):
+        v = str_to_int(v)
+    return _cy.vertex_to_latlng(v)


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

ajfriend · 2024-08-24T22:24:32Z

src/h3/api/basic_int/__init__.py

+        if isinstance(v, str):
+            v = str_to_int(v)
+        return _cy.is_valid_vertex(v)


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

ajfriend · 2024-08-24T22:27:20Z

tests/test_h3.py

+
+def test_is_valid_vertex():
+    assert h3.is_valid_vertex('2114c3ffffffffff')
+    assert h3.is_valid_vertex(2455495337847029759)


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

ajfriend · 2024-08-24T22:28:09Z

Added some minor comments, but overall this looks great. Thanks!

ajfriend · 2024-08-24T22:29:49Z

tests/test_h3.py

+    latlng = h3.vertex_to_latlng('2114c3ffffffffff')
+    assert latlng == approx((24.945215618732814, -70.33904370008679))
+    latlng = h3.vertex_to_latlng(2455495337847029759)


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).

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

ajfriend · 2024-08-25T02:46:39Z

src/h3/api/basic_int/__init__.py

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


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

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

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

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

Oh I see, thank you! Fixed.

Shiran-Yuan · 2024-08-25T06:12:00Z

Update: All previously proposed comments by @ajfriend have now been resolved! :-)

isaacbrodsky

@ajfriend Coverage test seems to be failing due to a Codecov token issue, is that on your radar?

isaacbrodsky · 2024-08-25T16:19:14Z

src/h3/api/basic_int/__init__.py

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


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

ajfriend · 2024-08-25T17:25:58Z

While I'm trying to fix the codedov issue, @Shiran-Yuan, would you mind also adding these functions to the docs? It should just involve creating a new section for these vertex functions in https://github.com/uber/h3-py/blob/master/docs/api_quick.md

Shiran-Yuan · 2024-08-25T17:28:04Z

While I'm trying to fix the codedov issue, @Shiran-Yuan, would you mind also adding these functions to the docs? It should just involve creating a new section for these vertex functions in https://github.com/uber/h3-py/blob/master/docs/api_quick.md

I see, no problem, I'll go add that.

ajfriend

This looks good to me! Thanks for the PR! I think we can land without the codecov since it isn't clear when that will get resolved. I'll leave this up for a bit to see if anyone else has comments.

* Update CHANGELOG.md * Update h3lib.pxd * Update latlng.pxd * Update latlng.pyx * Update util.pxd * Update util.pyx * Update _version.py * Update __init__.py * Update __init__.py * Update test_h3.py * Update __init__.py * Update test_h3.py * Update CHANGELOG.md * Update __init__.py * Update test_h3.py * Update _version.py * Update test_h3.py * Update __init__.py * Update CMakeLists.txt * Update latlng.pxd * Update latlng.pyx * Create vertex.pxd * Create vertex.pyx * Update __init__.py * Update test_h3.py * Update __init__.py * Update api_quick.md

* trying a scikit-build-core setup * trying things * vendor FindCython * fix build * remove unused * cython tests working * fix lint action * coverage fix * drop annotation stuff for now, because i can't get it working * book building * all target * try again * don't install all * numpy! * pip install h3.tar.gz[test] * drop wheel builds for 3.6 and 3.7 * move coverage config to pyproject.toml * remove requirements-dev.txt * remove cmake from the build system requirements * single sourcing the version number in _version.py * i think scikitbuild-core doesn't work with dynamic = ['version']. try something else * uber's * alphabetical files in cmakelists.txt * tidy up files in the sdist * move pytest config to pyproject.toml * CIBW_TEST_COMMAND: pytest * CIBW_TEST_COMMAND: pytest {project}/tests * CIBW_TEST_REQUIRES: pytest pytest-cov numpy * get better coverage on the testing code * fix api reference link (#385) * Added Bindings for Vertex-Related Functions (#388) * Update CHANGELOG.md * Update h3lib.pxd * Update latlng.pxd * Update latlng.pyx * Update util.pxd * Update util.pyx * Update _version.py * Update __init__.py * Update __init__.py * Update test_h3.py * Update __init__.py * Update test_h3.py * Update CHANGELOG.md * Update __init__.py * Update test_h3.py * Update _version.py * Update test_h3.py * Update __init__.py * Update CMakeLists.txt * Update latlng.pxd * Update latlng.pyx * Create vertex.pxd * Create vertex.pyx * Update __init__.py * Update test_h3.py * Update __init__.py * Update api_quick.md * Bump actions/download-artifact from 4.1.4 to 4.1.7 in /.github/workflows (#392) Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 4.1.4 to 4.1.7. - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](actions/download-artifact@v4.1.4...v4.1.7) --- updated-dependencies: - dependency-name: actions/download-artifact dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * prep for v4.0.0b6 release (#394) * alphabetical files in cmakelists.txt * single * test with numpy>=2 * drop lower bound on numpy version --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Isaac Brodsky <isaac@isaacbrodsky.com> Co-authored-by: Pieter Provoost <pieterprovoost@gmail.com> Co-authored-by: Shiran Yuan <shiran.yuan@outlook.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Shiran-Yuan added 12 commits August 23, 2024 17:51

Update CHANGELOG.md

3d07598

Update h3lib.pxd

87f749e

Update latlng.pxd

ec1d546

Update latlng.pyx

91c837c

Update util.pxd

291b02d

Update util.pyx

469ac00

Update _version.py

b202241

Update __init__.py

08d2621

Update __init__.py

f69b46d

Update test_h3.py

0a06d75

Update __init__.py

a788a2b

Update test_h3.py

792700c

ajfriend reviewed Aug 24, 2024

View reviewed changes

ajfriend requested review from nrabinowitz, dfellis and isaacbrodsky August 24, 2024 22:28

ajfriend reviewed Aug 24, 2024

View reviewed changes

Shiran-Yuan added 4 commits August 24, 2024 16:47

Update CHANGELOG.md

681a241

Update __init__.py

de57f68

Update test_h3.py

863123a

Update _version.py

a7fa37f

Update test_h3.py

f367655

ajfriend reviewed Aug 25, 2024

View reviewed changes

Shiran-Yuan added 8 commits August 25, 2024 06:05

Update __init__.py

d1a2031

Update CMakeLists.txt

7d46922

Update latlng.pxd

06e271b

Update latlng.pyx

06f96e8

Create vertex.pxd

b894274

Create vertex.pyx

149ee2c

Update __init__.py

dd272ca

Update test_h3.py

b8ecc81

isaacbrodsky reviewed Aug 25, 2024

View reviewed changes

Update __init__.py

e15f34a

isaacbrodsky approved these changes Aug 25, 2024

View reviewed changes

Update api_quick.md

b73d6ab

ajfriend changed the title ~~[Tentative Version 4.0.0b6] Added Bindings for Vertex-Related Functions~~ Added Bindings for Vertex-Related Functions Aug 25, 2024

ajfriend approved these changes Aug 25, 2024

View reviewed changes

ajfriend merged commit 46a7181 into uber:master Aug 27, 2024
40 of 41 checks passed

This was referenced Aug 27, 2024

Vertex Functions Still Not Binded... #386

Closed

Bind vertex functions #323

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Bindings for Vertex-Related Functions #388

Added Bindings for Vertex-Related Functions #388

Shiran-Yuan commented Aug 24, 2024 •

edited

Loading

CLAassistant commented Aug 24, 2024 •

edited

Loading

ajfriend commented Aug 24, 2024

ajfriend Aug 24, 2024

Shiran-Yuan Aug 24, 2024

ajfriend Aug 24, 2024

Shiran-Yuan Aug 24, 2024

ajfriend Aug 25, 2024

Shiran-Yuan Aug 25, 2024

ajfriend Aug 24, 2024

Shiran-Yuan Aug 24, 2024

ajfriend Aug 24, 2024

Shiran-Yuan Aug 24, 2024

ajfriend Aug 24, 2024

Shiran-Yuan Aug 24, 2024

ajfriend Aug 24, 2024

Shiran-Yuan Aug 24, 2024

ajfriend Aug 24, 2024

Shiran-Yuan Aug 24, 2024

ajfriend commented Aug 24, 2024

ajfriend Aug 24, 2024

Shiran-Yuan Aug 24, 2024

ajfriend Aug 25, 2024

isaacbrodsky Aug 25, 2024

Shiran-Yuan Aug 25, 2024

isaacbrodsky Aug 25, 2024

Shiran-Yuan Aug 25, 2024

Shiran-Yuan commented Aug 25, 2024

isaacbrodsky left a comment

isaacbrodsky Aug 25, 2024

ajfriend commented Aug 25, 2024

Shiran-Yuan commented Aug 25, 2024

ajfriend left a comment

		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.

Added Bindings for Vertex-Related Functions #388

Added Bindings for Vertex-Related Functions #388

Conversation

Shiran-Yuan commented Aug 24, 2024 • edited Loading

CLAassistant commented Aug 24, 2024 • edited Loading

ajfriend commented Aug 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajfriend commented Aug 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shiran-Yuan commented Aug 25, 2024

isaacbrodsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajfriend commented Aug 25, 2024

Shiran-Yuan commented Aug 25, 2024

ajfriend left a comment

Choose a reason for hiding this comment

Shiran-Yuan commented Aug 24, 2024 •

edited

Loading

CLAassistant commented Aug 24, 2024 •

edited

Loading