Conversation
| <TabItem value="python"> | ||
|
|
||
| ```py | ||
| h3.get_index_digit(h, digit) | ||
| ``` | ||
|
|
||
| </TabItem> | ||
| <TabItem value="go"> | ||
|
|
||
| ```go | ||
| cell.IndexDigit(digit) | ||
| ``` | ||
|
|
||
| </TabItem> | ||
| <TabItem value="duckdb"> | ||
|
|
||
| ```sql | ||
| h3_get_index_digit(h, digit) | ||
| ``` | ||
|
|
||
| </TabItem> |
There was a problem hiding this comment.
Are we sure we want to add these to the docs before they're actually implemented in the bindings?
There was a problem hiding this comment.
I think it's helpful to show what it would look like in the bindings, and I would expect the bindings update very quickly to have these functions as they're quite simple.
| int H3_EXPORT(getReservedBits)(H3Index h) { return H3_GET_RESERVED_BITS(h); } | ||
|
|
||
| /** | ||
| * Returns the index digits at `digit`, which starts with 1 for resolution |
There was a problem hiding this comment.
I would use "resolution" here to specify the function input, and "digit" as the function output. I think that's more in line with how we use resolution elsewhere.
From the docs:
This is followed by a sequence of r digits 0-6, where each ith digit di specifies one of the 7 cells
centered on the cell indicated by the coarser resolution digits d1 through di-1. A local hexagon
coordinate system is assigned to each of the resolution 0 base cells and is used to orient all
hierarchical indexing child cells of that base cell. The assignment of digits 0-6 at each resolution
uses a Central Place Indexing arrangement (see [Sahr, 2014](http://webpages.sou.edu/~sahrk/sqspc/pubs/autocarto14.pdf))
But maybe that's just me. What do others think?
There was a problem hiding this comment.
Generally, a variable named res or resolution indicates to the user that it is an integer taking values 0--15. (In this case, its a little more restrictive at 1--15.) And digit indicates something taking values 0--6 (or 7 to indicate an invalid digit). I'd keep that same convention here.
Elsewhere in the codebase, we use res as the second argument to the H3_GET_INDEX_DIGIT macro, like here:
Line 227 in 73cf911
There was a problem hiding this comment.
As does the macro definition:
h3/src/h3lib/include/h3Index.h
Line 140 in 73cf911
There was a problem hiding this comment.
Updated to rename that parameter to res.
|
This would help resolve #277 (the inspection part). |
ajfriend
left a comment
There was a problem hiding this comment.
Looks good to me, thanks!
I have some non-blocking questions about the Doxygen comments, but we can handle those separately, since they're not particular to this PR---I'm just noticing them now. Basically, it looks like we generally split Doxygen comment information across .h and .c files. Should we combine them to a single source of truth (e.g., the .h file)? Or is there a reason we separate them?
| * @param h The H3 index. | ||
| * @return The reserved bits of the H3 index argument. | ||
| */ | ||
| int H3_EXPORT(getReservedBits)(H3Index h) { return H3_GET_RESERVED_BITS(h); } |
There was a problem hiding this comment.
Do we have a better name here?
|
I updated this PR to not include getReservedBits as I wasn't sure about that name. |
nrabinowitz
left a comment
There was a problem hiding this comment.
Couple of documentation comments, otherwise LGTM
| Returns an indexing digit of the index. | ||
| Works for cells, edges and vertexes. |
There was a problem hiding this comment.
Does this need more info about what an indexing digit is, or a link to the bit layout docs?
There was a problem hiding this comment.
This could certainly use more content. Also please note I hid the other bindings until this version of H3 is released.
#1022
This adds functions for retrieving index digits and the reserved bits from indexes. The reserved bits may be of interest for directedEdge / vertex mode where knowing which one it is are helpful.
Note that I chose to allow getIndexDigit to read unused digits; we could instead choose to disallow reading those digits from this function as they (probably) shouldn't be used as three-wide bit fields in that case.
Does not have CLI tests just yet.