8000 feat: Implement gridRing by justinhwang · Pull Request #1011 · uber/h3 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: Implement gridRing #1011

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 5, 2025
Merged

feat: Implement gridRing #1011

merged 1 commit into from
Jun 5, 2025

Conversation

justinhwang
Copy link
Contributor

This commit introduces a new public API function, gridRing, to
retrieve H3 cells located at an exact grid distance k from an origin
cell, forming a "hollow" ring.

The implementation mirrors implementation of gridDiskDistances
wherein we first attempt to use the unsafe version. If the unsafe
version fails, we fallback to _gridRingInternal.

_gridRingInternal utilizes _gridDiskDistancesInternal to get all
cells up to distance k, then filters the output to only include those
at exactly k.

@justinhwang justinhwang force-pushed the justinhwang/gridring branch 3 times, most recently from e4bbec6 to d340990 Compare June 4, 2025 19:36
@justinhwang
Copy link
Contributor Author

Not quite sure why were getting these errors

/home/runner/work/h3/h3/src/apps/testapps/testGridRing.c:32:35: error: call to undeclared function 'gridRing'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   32 |         t_assertSuccess(H3_EXPORT(gridRing)(sfHex, 0, k0));
      |

when the test file has:

#include "h3api.h"

also my first time writing in C so any help is appreciated!

@isaacbrodsky
Copy link
Collaborator

Not quite sure why were getting these errors

/home/runner/work/h3/h3/src/apps/testapps/testGridRing.c:32:35: error: call to undeclared function 'gridRing'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   32 |         t_assertSuccess(H3_EXPORT(gridRing)(sfHex, 0, k0));
      |

when the test file has:

#include "h3api.h"

also my first time writing in C so any help is appreciated!

The issue is because the declaration needs to be added to h3api.h.in (which gets processed into h3api.h), otherwise the compiler isn't sure what that function looks like.

@justinhwang justinhwang force-pushed the justinhwang/gridring branch from d340990 to dceed5f Compare June 4, 2025 20:17
@isaacbrodsky
Copy link
Collaborator

Thanks for this contribution! I will take a look in more detail.

If you're comfortable, could you also add this function to fuzzerGridDisk so we get the full fuzzer coverage? (We can also add that in a follow up)

@justinhwang
Copy link
Contributor Author

The issue is because the declaration needs to be added to h3api.h.in (which gets processed into h3api.h), otherwise the compiler isn't sure what that function looks like.

Ah I didn't notice that src/h3lib/include/h3api.h was local and in .gitignore, thanks!

Thanks for this contribution! I will take a look in more detail.

If you're comfortable, could you also add this function to fuzzerGridDisk so we get the full fuzzer coverage? (We can also add that in a follow up)

Would prefer if someone could help me do that in a followup, thank you :)

@coveralls
Copy link
coveralls commented Jun 4, 2025 •
edited
Loading

Coverage Status

coverage: 98.804% (+0.02%) from 98.786%
when pulling 12fd76a on justinhwang:justinhwang/gridring
into b70bd72 on uber:master.

@shresss shresss mentioned this pull request Jun 4, 2025
Closed
dfellis
dfellis reviewed Jun 4, 2025
View reviewed changes
@justinhwang justinhwang force-pushed the justinhwang/gridring branch from dceed5f to 12fd76a Compare June 4, 2025 22:46
justinhwang
justinhwang commented Jun 4, 2025
View reviewed changes
@isaacbrodsky isaacbrodsky merged commit 6be0899 into uber:master Jun 5, 2025
45 checks passed
@isaacbrodsky isaacbrodsky mentioned this pull request Jun 5, 2025
Merged
shresss added a commit to shresss/h3 that referenced this pull request Jun 5, 2025
@shresss
feat(fuzzer): Add gridRing test to fuzzerGridDisk
b390e40 
Adds gridRing testing to fuzzerGridDisk.c to improve test coverage
as requested in uber#1011 and as I tried but ran into merge issues with in uber#1012.
@shresss shresss mentioned this pull request Jun 5, 2025
Merged
@ajfriend ajfriend mentioned this pull request Jun 18, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfellis dfellis dfellis approved these changes

@isaacbrodsky isaacbrodsky isaacbrodsky approved these changes

@nrabinowitz nrabinowitz Awaiting requested review from nrabinowitz

@ajfriend ajfriend Awaiting requested review from ajfriend

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@justinhwang @isaacbrodsky @coveralls @dfellis
0