-
Notifications
You must be signed in to change notification settings - Fork 459
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
Feature: Add cellToChildPos and childPosToCell #719
Conversation
src/h3lib/lib/h3Index.c
Outdated
// different from hexagons | ||
for (int res = childRes; res > parentRes; res--) { | ||
// It shouldn't be possible to get an error here, so we don't check | ||
H3_EXPORT(cellToParent(child, res - 1, &parent)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a good spot for NEVER
once #720 lands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand how these functions are used so they look helpful.
Please add website API documentation for these functions.
@@ -0,0 +1,95 @@ | |||
/* | |||
* Copyright 2017-2021 Uber Technologies, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Copyright 2017-2021 Uber Technologies, Inc. | |
* Copyright 2022 Uber Technologies, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Copyright 2017-2021 Uber Technologies, Inc. | |
* Copyright 2017-2022 Uber Technologies, Inc. |
} | ||
} | ||
|
||
return E_SUCCESS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the passed in childPos is abnormally high or low? E.g. if idx > 0
here or if childPos < 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another good question. You'd get an invalid digit for output; I'm not sure there would be other issues. Probably worth validating childPos
and returning an error for bad cases.
H3_EXPORT(cellToParent(child, res - 1, &parent)); | ||
parentIsPentagon = H3_EXPORT(isPentagon)(parent); | ||
int rawDigit = H3_GET_INDEX_DIGIT(child, res); | ||
int digit = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens for invalid indexes into the deleted K subsequence of a pentagon, or indexes with INVALID_DIGIT (7)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good questions:
- Invalid indexes into the K subsequence will be treated as
0
per the logic on the next line - INVALID_DIGIT will result in an offset that's too high, and the resulting number might be greater than the number of max children
This might be okay, on the principle of GIGO, but I could imagine attacks on systems using H3 that would use poor output here to trigger random memory access. Possible failsafes:
- We could validate the digit and return an error
- We could clamp output to the max possible child index for the given parent
Validating the digits is probably worthwhile. If we do that we don't need to clamp output, but we could (after #720) add an assertion at the end of the function like
if (NEVER(out > cellToChildrenSize(parent, childRes))) {
return E_FAILED;
}
* children at the specified resolution */ | ||
H3Error H3_EXPORT(childPosToCell)(int64_t childPos, H3Index parent, | ||
int childRes, H3Index *child) { | ||
if (childRes < 0 || childRes > MAX_H3_RES) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also be a good place for testcase
from #720 to ensure boundary conditions work as expected.
8b48a58
to
ba5ed53
Compare
<TabItem value="c"> | ||
|
||
```c | ||
H3Index cellToChildPos(H3Index child, int parentRes, int64_t *out); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the other bindings say "cell" - maybe they should also have the parameter named "child"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all prospective, of course, as the binding owners may have other ideas, but I can update all to child
for now
int64_t maxChildCount; | ||
H3Error sizeError = | ||
H3_EXPORT(cellToChildrenSize)(parent, childRes, &maxChildCount); | ||
if (NEVER(sizeError)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, we should require fuzzers be added too to try to exercise these. I am fine with merging this without the fuzzer and then adding it in a follow up PR.
Adds two new functions:
cellToChildPos
gives the position of the child in an ordered list of all the children of the cell's parent at a given resolution. The "ordered list" here is the same order as returned bycellToChildren
, which I believe is numerically sorted by index. Essentially, this function allows you to determine the numeric index of the cell among its siblings, given a parent resolution that defines the sibling list.childPosToCell
offers the reverse functionality, returning a child cell given a parent, a child resolution, and a child index.This pair of functions opens up several new use cases:
cellToChildren
. I think we can do this internally in the library using iterators, but this offers an ergonomic API for external consumers (simply iteratefor (int i = 0; i < cellToChildrenSize(parent, res); i++)
and takechildPosToCell(i, parent, res)
).uint16
if the parent/child res difference is <= 5) and bit arrays (e.g. 16807 bits / 2101 bytes to indicate yes/no whether each child 5 steps down is included in the set). This is likely more effective thancompact
if there's no particular likelihood that the cells are contiguous.Notes
childPosition
might be clearer (but longer),childIndex
makes sense but risked confusion with H3 indexes.