10000 feat: add GridRing and GridRingUnsafe by justinhwang · Pull Request #82 · uber/h3-go · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: add GridRing and GridRingUnsafe #82

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 4, 2025
Merged

feat: add GridRing and GridRingUnsafe #82

merged 1 commit into from
Jun 4, 2025

Conversation

justinhwang
Copy link
Collaborator
@justinhwang justinhwang commented Jun 3, 2025 •
edited
Loading

Add gridRing and gridRingUnsafe for hollow ring functionality.

This was in v3 as HexRing. Though the v3 to v4 migration guide notes
that there should be a gridRingSafe and gridRing (which calls both
the Safe and Unsafe variants), neither exists in the base C lib. Use
gridDiskDistance for gridRing instead.

@CLAassistant
Copy link
CLAassistant commented Jun 3, 2025 •
edited
< 8000 /summary> Loading

CLA assistant check
All committers have signed the CLA.

@justinhwang
Copy link
Collaborator Author

@jogly @zachcoleman think you folks could take a look? thanks!

@zachcoleman
Copy link
Collaborator

@justinhwang I think a better approach would be to add a GridRing function that follows the approach taken in the Python library and other bindings. This typically calls GridRingUnsafe then falls back to another calculation based on GridDisk + filtering. Thoughts? Including an unsafe binding to the API feels odd to me.

Ref:
https://uber.github.io/h3-py/_modules/h3/api/basic_str.html#grid_ring
https://github.com/uber/h3-py/blob/856d481de31c54aaa6076020ade6b88ed4d316cc/src/h3/_cy/cells.pyx#L90-L140

@justinhwang
Copy link
Collaborator Author

@zachcoleman Yeah I had the same question as I was implementing this. I ended up implementing it because from the v3 to v4 migration guide, it seems like we should have both a GridRing and a GridRingUnsafe Go binding (while the Safe variant is noted in the guide that they won't be exposed unless there is a need to).

It looks like the base C library is missing the gridRing and gridRingSafe variants so we could potentially proceed with this one as we get the others added to the base C library. Lmk what you think!

@jogly
Copy link
Collaborator
jogly commented Jun 3, 2025

I agree @zachcoleman; in isolation I would probably do as in this PR, but I think consistency with the other bindings is more valuable for the ecosystem.

@justinhwang
Copy link
Collaborator Author

re: consistency with other bindings - should we implement gridRing and gridRingSafe in the C lib so bindings where we reference C don't need to reimplement?

@zachcoleman
Copy link
Collaborator
zachcoleman commented Jun 3, 2025 •
edited
Loading

re: consistency with other bindings - should we implement gridRing and gridRingSafe in the C lib so bindings where we reference C don't need to reimplement?

I'm sure they're open to PRs as well. I think taking the Python repo's lead, it could be pretty straight forward for us to provide GridRing on the current C API s.t. users don't necessarily need to be concerned nor incur a performance penalty in a vast majority of cases.

Moving forward, if you or someone added the C implementations, we can always cut a release and update to the new underlying version as well.

@justinhwang
Copy link
Collaborator Author

@zachcoleman yeah that makes sense - should we expose both GridRing and GridRingUnsafe for completeness? (wherein GridRing is a followup PR)

@zachcoleman
Copy link
Collaborator

@zachcoleman yeah that makes sense - should we expose both GridRing and GridRingUnsafe for completeness? (wherein GridRing is a followup PR)

I don't think it overloads this PR to include GridRing here.

@justinhwang justinhwang force-pushed the justinhwang/gridring branch from 57cbc4c to 9d956ac Compare June 3, 2025 22:38
@justinhwang
Copy link
Collaborator Author

Updated @zachcoleman

@justinhwang justinhwang force-pushed the justinhwang/gridring branch from 9d956ac to a084e1c Compare June 3, 2025 22:42
@justinhwang justinhwang changed the title feat: add GridRingUnsafe feat: add GridRing and GridRingUnsafe Jun 3, 2025
zachcoleman
zachcoleman approved these changes Jun 4, 2025
View reviewed changes
Copy link
Collaborator
@zachcoleman zachcoleman left a comment

Choose a reason for hiding this comment

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

This looks great to me

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

Pull Request Test Coverage Report for Build 15429331645

Details

  • 22 of 22 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 98.578%
Totals Coverage Status
Change from base Build 14172792260: 0.05%
Covered Lines: 624
Relevant Lines: 633
💛 - Coveralls

jogly
jogly reviewed Jun 4, 2025
View reviewed changes
@zachcoleman zachcoleman merged commit a2a9982 into uber:master Jun 4, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jogly jogly jogly approved these changes

@zachcoleman zachcoleman zachcoleman approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@justinhwang @CLAassistant @zachcoleman @jogly @coveralls
0