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

Implement h3GetFaces #253

Merged
merged 16 commits into from Jul 22, 2019
Merged

Implement h3GetFaces #253

merged 16 commits into from Jul 22, 2019

Conversation

@nrabinowitz
Copy link
Contributor

nrabinowitz commented Jul 15, 2019

Implements h3GetFaces per the discussion in #236, as well as the helper function maxFaceCount (using the same pattern as maxKRingSize etc). The h3GetFaces function takes an H3 index and an output array and fills the output array with all the icosahedron faces the index intersects. It returns 1 or 2 faces for hexagons and 5 faces for pentagons.

Implementing this required breaking up _faceIjkToGeoBoundary and _faceIjkPentToGeoBoundary in order to reuse the logic for getting the FaceIJK coords of the vertices - I've checked the benchmark locally and don't see any difference in h3ToGeoBoundary performance.

Also adds the test helper iterateBaseCellIndexesAtRes to iterate over a single specified base cell.

Closes #236

/** Invalid faceNeighbors table direction */
#define INVALID -1
/** Center faceNeighbors table direction */
#define CENTER 0

This comment has been minimized.

Copy link
@nrabinowitz

nrabinowitz Jul 15, 2019

Author Contributor

Unused

src/h3lib/lib/faceijk.c Outdated Show resolved Hide resolved
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 15, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling eb178d7 on nrabinowitz:h3-get-faces into 1b049bb on uber:master.

Copy link
Collaborator

dfellis left a comment

Minor comments, though that other ticket about the memory allocator being swappable also implies that some of the functions here (and throughout the codebase) should be audited for static memory allocations that break parallelization since just changing the allocator would not be enough to solve that ticket.

src/h3lib/lib/h3Index.c Show resolved Hide resolved
_h3ToFaceIjk(h3, &fijk);

// Get all vertices as FaceIJK addresses. For simplicity, always
// initialize the array with 6 verts, ignoring the last one for pentagons

This comment has been minimized.

Copy link
@dfellis

dfellis Jul 15, 2019

Collaborator

Nit: This array isn't initialized, you've literally told the C compiler to leave the space for 6 structs directly allocated in the library for this function to use for this operation. (Which also makes this method not-parallel-computing-friendly.)

This comment has been minimized.

Copy link
@nrabinowitz

nrabinowitz Jul 15, 2019

Author Contributor

Sorry, array handling is still the most confusing part for me of working in C. You're saying I should use malloc here?

This comment has been minimized.

Copy link
@nrabinowitz

nrabinowitz Jul 15, 2019

Author Contributor

Or would this work with FaceIJK fijkVerts[NUM_HEX_VERTS] = {0}?

This comment has been minimized.

Copy link
@dfellis

dfellis Jul 15, 2019

Collaborator

The first one. The memory needs to be allocated either on the heap or on the stack. This syntax tells the compiler to put some dedicated space somewhere in the .so/.dll file and the function then references that space for all calls.

If multiple threads are running the same function at the "same time" (either actually at the same time on different cores, or one thread was running and a CPU interrupt happened partway through and the kernel scheduler spun up another thread and it continued in the same function) you could have very-hard-to-track bugs where the function gives you the wrong answer a vanishingly small amount of the time.

This comment has been minimized.

Copy link
@nrabinowitz

nrabinowitz Jul 15, 2019

Author Contributor

Got it. I was expecting the array allocation here to work the same as allocating a struct (which doesn't do what you're describing, right?).

This comment has been minimized.

Copy link
@dfellis

dfellis Jul 16, 2019

Collaborator

Mostly academic, yes, but if the compiler can do the allocation in stack space and not cause iOS to barf, that would be faster (and more predictable in speed) than malloc, which will sometimes have to call out to the kernel to get a new page of memory and cause an interrupt.

I would say the malloc is definitely safer with so many compile targets to worry about.

This comment has been minimized.

Copy link
@isaacbrodsky

isaacbrodsky Jul 19, 2019

Collaborator

I don't believe that 6 FaceIJK structures would be an issue, so I'd suggest using stack allocation here. I don't think we need to be quite that careful about stack space.

This comment has been minimized.

Copy link
@nrabinowitz

nrabinowitz Jul 19, 2019

Author Contributor

Is FaceIJK fijkVerts[NUM_HEX_VERTS] = {0} using stack allocation, then? Is that the suggested change?

This comment has been minimized.

Copy link
@nrabinowitz

nrabinowitz Jul 20, 2019

Author Contributor

Updated to FaceIJK fijkVerts[NUM_HEX_VERTS] = {0} per @isaac's suggestion.

This comment has been minimized.

Copy link
@nrabinowitz

nrabinowitz Jul 20, 2019

Author Contributor

The previous formulation had compiler warnings, so I'm back to my original FaceIJK fijkVerts[NUM_HEX_VERTS]; implementation. The internet seems to think this is a stack allocation, and @dfellis's original concern wasn't valid (unless this was declared at the file level and/or static). Please let me know if there are additional concerns here.

src/h3lib/lib/h3Index.c Outdated Show resolved Hide resolved
src/h3lib/lib/faceijk.c Outdated Show resolved Hide resolved
src/h3lib/lib/h3Index.c Outdated Show resolved Hide resolved
@nrabinowitz nrabinowitz force-pushed the nrabinowitz:h3-get-faces branch from fadb921 to 5c5f986 Jul 17, 2019
src/apps/testapps/testH3GetFaces.c Outdated Show resolved Hide resolved
src/apps/testapps/testH3GetFaces.c Outdated Show resolved Hide resolved
src/h3lib/lib/faceijk.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 Show resolved Hide resolved
_h3ToFaceIjk(h3, &fijk);

// Get all vertices as FaceIJK addresses. For simplicity, always
// initialize the array with 6 verts, ignoring the last one for pentagons

This comment has been minimized.

Copy link
@isaacbrodsky

isaacbrodsky Jul 19, 2019

Collaborator

I don't believe that 6 FaceIJK structures would be an issue, so I'd suggest using stack allocation here. I don't think we need to be quite that careful about stack space.

@nrabinowitz nrabinowitz force-pushed the nrabinowitz:h3-get-faces branch from e22519a to eb178d7 Jul 22, 2019
src/h3lib/lib/h3Index.c Show resolved Hide resolved
@nrabinowitz nrabinowitz merged commit 4f49fea into uber:master Jul 22, 2019
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
license/cla Contributor License Agreement is signed.
Details
@nrabinowitz nrabinowitz deleted the nrabinowitz:h3-get-faces branch Jul 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.