Skip to content

constructCell#1063

Merged
ajfriend merged 113 commits intouber:masterfrom
ajfriend:cell_from_components.clean
Oct 18, 2025
Merged

constructCell#1063
ajfriend merged 113 commits intouber:masterfrom
ajfriend:cell_from_components.clean

Conversation

@ajfriend
Copy link
Copy Markdown
Collaborator

@ajfriend ajfriend commented Oct 14, 2025

Taking over from #1037.

  • constructCell constructs a valid h3 cell from components (resolution, base cell, digits).
  • Adds a few relevant error codes: E_BASE_CELL_DOMAIN, E_DIGIT_DOMAIN, E_DELETED_DIGIT
  • Uses a "table of tests" strategy to make test cases and failures more obvious.

subtasks:

@ajfriend ajfriend changed the title Cell from components (retry) constructCell Oct 14, 2025
@ajfriend ajfriend changed the title constructCell constructCell Oct 14, 2025
Comment thread src/apps/testapps/testDescribeH3Error.c Outdated
Comment thread src/apps/testapps/testConstructCell.c Outdated
Comment thread src/apps/testapps/testConstructCell.c Outdated
Comment thread src/apps/testapps/testConstructCell.c
Comment thread src/h3lib/lib/h3Index.c Outdated
Comment thread src/h3lib/include/h3api.h.in Outdated
@ajfriend ajfriend mentioned this pull request Oct 15, 2025
Comment thread src/h3lib/include/h3api.h.in
Comment thread src/h3lib/lib/h3Index.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
bool isPentagon = isBaseCellPentagonArr[baseCellNumber];

for (int r = 1; r <= res; r++) {
int d = digits[r - 1];
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.

Practically speaking, the user can pass in an array of length res - 1. Is this intended behavior and part of the function contract, or must a user always pass an array of length 15?

Copy link
Copy Markdown
Collaborator Author

@ajfriend ajfriend Oct 16, 2025

Choose a reason for hiding this comment

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

The user doesn't need to pass in an array of length 15 unless they're making a res 15 cell.

The function just takes in a pointer, which we assume points to an array of length res:

H3Error constructCell(int res, int baseCellNumber, int *digits, H3Index *out);

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.

We will need to document that for the C API, as users will make shorter arrays and the function must never attempt to index beyond the end of the array. I would include that in the docstring for this, when describing what the int *digits parameter means.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added docs in h3Index.c, which is what we typically do.

Thought: Wouldn't it make more sense to document function params and other things relevant to the API in h3api.h.in? I forget if we have a historical reason for why we tend to keep the documentation fairly light in the header file, but more detailed in the implementation. It makes sense to keep algorithm descriptions and internal details there, but I'd think we'd want the API spec in the header, right? cc: @isaacbrodsky @dfellis @nrabinowitz

But I might be forgetting why we do it this way. And this is definitely a discussion we can continue elsewhere.

@ajfriend ajfriend requested a review from dfellis October 17, 2025 01:29
@ajfriend ajfriend requested a review from dmitryzv October 17, 2025 18:01
@ajfriend ajfriend merged commit 1a5adac into uber:master Oct 18, 2025
45 checks passed
@ajfriend ajfriend deleted the cell_from_components.clean branch October 18, 2025 12:54
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.

5 participants