Skip to content

Add h3ToCenterChild and getPentagonsAtRes#267

Merged
nrabinowitz merged 21 commits into
uber:masterfrom
zachasme:feature-add-h3tocenterchild
Aug 7, 2019
Merged

Add h3ToCenterChild and getPentagonsAtRes#267
nrabinowitz merged 21 commits into
uber:masterfrom
zachasme:feature-add-h3tocenterchild

Conversation

@zachasme

@zachasme zachasme commented Aug 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

coveralls commented Aug 1, 2019 •
edited
Loading

Copy link
Copy Markdown

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

zachasme commented Aug 1, 2019

Copy link
Copy Markdown
Contributor Author

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
nrabinowitz
nrabinowitz reviewed Aug 1, 2019
View reviewed changes

@nrabinowitz nrabinowitz left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread docs/api/hierarchy.md Outdated
Comment thread src/apps/testapps/testH3ToCenterChild.c
Comment thread src/apps/testapps/testPentagonsAtRes.c Outdated
Comment thread src/apps/testapps/testPentagonsAtRes.c Outdated
Comment thread src/h3lib/lib/h3Index.c Outdated
Comment thread src/h3lib/lib/h3Index.c Outdated
Comment thread src/h3lib/lib/h3Index.c
Comment thread src/h3lib/lib/h3Index.c Outdated
@nrabinowitz

nrabinowitz commented Aug 1, 2019

Copy link
Copy Markdown
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.

isaacbrodsky
isaacbrodsky reviewed Aug 1, 2019
View reviewed changes

@isaacbrodsky isaacbrodsky left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread docs/api/hierarchy.md Outdated
Comment thread docs/api/misc.md Outdated
Comment thread docs/api/misc.md Outdated
Comment thread src/apps/testapps/testH3ToCenterChild.c Outdated
Comment thread src/apps/testapps/testPentagonsAtRes.c Outdated
Comment thread src/apps/testapps/testH3ToCenterChild.c
Comment thread src/apps/testapps/testPentagonsAtRes.c Outdated
Comment thread src/h3lib/lib/h3Index.c Outdated
Comment thread src/h3lib/lib/h3Index.c Outdated
nrabinowitz
nrabinowitz reviewed Aug 1, 2019
View reviewed changes
Comment thread src/apps/testapps/testH3ToCenterChild.c Outdated
@zachasme

zachasme commented Aug 2, 2019

Copy link
Copy Markdown
Contributor Author

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.

isaacbrodsky
isaacbrodsky reviewed Aug 2, 2019
View reviewed changes
Comment thread src/apps/testapps/testCompact.c Outdated
Comment thread src/apps/testapps/testH3ToCenterChild.c
Comment thread src/h3lib/lib/h3Index.c Outdated
nrabinowitz
nrabinowitz reviewed Aug 2, 2019
View reviewed changes
Comment thread docs/api/misc.md Outdated
Comment thread src/apps/testapps/testCompact.c Outdated
Comment thread src/apps/testapps/testH3ToCenterChild.c
Comment thread src/apps/testapps/testPentagonIndexes.c Outdated
isaacbrodsky
isaacbrodsky approved these changes Aug 6, 2019
View reviewed changes
Comment thread src/h3lib/lib/h3Index.c Outdated
nrabinowitz
nrabinowitz approved these changes Aug 7, 2019
View reviewed changes

@nrabinowitz nrabinowitz left a comment

Copy link
Copy Markdown
Collaborator

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
Version 3.5.1, add CHANGELOG for uber#271, update to point to uber#267 …
2de2ba4 
…(the PR instead of issue)
@isaacbrodsky isaacbrodsky mentioned this pull request Aug 12, 2019
Merged
mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022
Merge pull request uber#267 from zachasme/feature-add-h3tocenterchild
d047f51 
Add h3ToCenterChild and getPentagonsAtRes
mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022
Version 3.5.1, add CHANGELOG for uber#271, update to point to uber#267 …
01ff652 
…(the PR instead of issue)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nrabinowitz nrabinowitz nrabinowitz approved these changes

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

add pentagonsAtRes function?

4 participants

@zachasme @coveralls @nrabinowitz @isaacbrodsky