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

Add h3ToCenterChild and getPentagonsAtRes #267

Merged
merged 21 commits into from
Aug 7, 2019

Conversation

zachasme
Copy link
Contributor

@zachasme zachasme commented Aug 1, 2019

Follows up on #263. I have a use-case where I need to find the center child of a cell at a specific resolution.

I'm simply changing the four resolution bits, but it should be worth adding as a helper function to the core library (so we can keep treating the index as an opaque datatype).

Also adds getPentagonsAtRes which provides the user with the set of pentagon cells at any given resolution,

Resolves #255 and #263.

@zachasme zachasme changed the title Add H3GetCenterChild Add H3ToCenterChild Aug 1, 2019
@coveralls
Copy link

coveralls commented Aug 1, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 4a4ad44 on zachasme:feature-add-h3tocenterchild into f6106a9 on uber:master.

@zachasme zachasme changed the title Add H3ToCenterChild Add h3ToCenterChild and getPentagonsAtRes Aug 1, 2019
@zachasme
Copy link
Contributor Author

zachasme commented Aug 1, 2019

What are your thoughts on naming?

The atRes suffix is not used anywhere else, perhaps getPentagonalIndexes/pentgonalIndexCount would be better?

@zachasme zachasme force-pushed the feature-add-h3tocenterchild branch from 2b4061b to 6b9d69c Compare August 1, 2019 11:26
Copy link
Collaborator

@nrabinowitz nrabinowitz left a comment

Choose a reason for hiding this comment

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

Thanks so much for the contribution! Looking really good, just a few comments.

docs/api/hierarchy.md Outdated Show resolved Hide resolved
src/apps/testapps/testH3ToCenterChild.c Show resolved Hide resolved
src/apps/testapps/testPentagonsAtRes.c Outdated Show resolved Hide resolved
src/apps/testapps/testPentagonsAtRes.c Outdated Show resolved Hide resolved
src/h3lib/lib/h3Index.c Outdated Show resolved Hide resolved
src/h3lib/lib/h3Index.c Outdated Show resolved Hide resolved
src/h3lib/lib/h3Index.c Show resolved Hide resolved
src/h3lib/lib/h3Index.c Outdated Show resolved Hide resolved
@nrabinowitz
Copy link
Collaborator

What are your thoughts on naming?

The atRes suffix is not used anywhere else, perhaps getPentagonalIndexes/pentgonalIndexCount would be better?

We also don't use pentagonal anywhere :). I don't mind atRes but could see just getPentagons or getPentagonIndexes. No strong preference.

Copy link
Collaborator

@isaacbrodsky isaacbrodsky left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! Looks good, I added some comments in addition to Nick's.

docs/api/hierarchy.md Outdated Show resolved Hide resolved
docs/api/misc.md Outdated Show resolved Hide resolved
docs/api/misc.md Outdated Show resolved Hide resolved
src/apps/testapps/testH3ToCenterChild.c Outdated Show resolved Hide resolved
src/apps/testapps/testPentagonsAtRes.c Outdated Show resolved Hide resolved
src/apps/testapps/testH3ToCenterChild.c Show resolved Hide resolved
src/apps/testapps/testPentagonsAtRes.c Outdated Show resolved Hide resolved
src/h3lib/lib/h3Index.c Outdated Show resolved Hide resolved
src/h3lib/lib/h3Index.c Outdated Show resolved Hide resolved
@zachasme
Copy link
Contributor Author

zachasme commented Aug 2, 2019

Thank you for all the feedback! I believe I've addressed it all. Please let me know if I've missed something.

I've added static bool _isValidChildRes(int, int) which is now used in h3ToChildren, maxH3ToChildrenSize, h3ToCenterChild, uncompact and maxUncompactSize as per your suggestion.

src/apps/testapps/testCompact.c Outdated Show resolved Hide resolved
src/apps/testapps/testH3ToCenterChild.c Show resolved Hide resolved
src/h3lib/lib/h3Index.c Outdated Show resolved Hide resolved
docs/api/misc.md Outdated Show resolved Hide resolved
src/apps/testapps/testCompact.c Outdated Show resolved Hide resolved
src/apps/testapps/testH3ToCenterChild.c Show resolved Hide resolved
src/apps/testapps/testPentagonIndexes.c Outdated Show resolved Hide resolved
src/h3lib/lib/h3Index.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@nrabinowitz nrabinowitz 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 - thanks for the follow-through on this PR!

@nrabinowitz nrabinowitz merged commit feff1dd into uber:master Aug 7, 2019
isaacbrodsky pushed a commit to isaacbrodsky/h3 that referenced this pull request Aug 12, 2019
@isaacbrodsky isaacbrodsky mentioned this pull request Aug 12, 2019
mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022
Add h3ToCenterChild and getPentagonsAtRes
mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add pentagonsAtRes function?
4 participants