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.

Sign up for GitHub

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

Jump to bottom

Fix for edge directions in getH3UnidirectionalEdgeBoundary #78

Merged
merged 6 commits into from
Jun 14, 2018
Merged

Fix for edge directions in getH3UnidirectionalEdgeBoundary #78

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a25f18c
Fix for edge directions in getH3UnidirectionalEdgeBoundary
Jun 12, 2018
622f617
Code review updates
Jun 12, 2018
ee30ce6
Comment updates, static and const, use STACK_ARRAY_ALLOC
Jun 13, 2018
e5b130e
Add const
Jun 13, 2018
e7001d9
Add requested comments about resolution
Jun 14, 2018
2d93522
make format
Jun 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 111 additions & 42 deletions src/apps/testapps/testH3UniEdge.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@

#include <stdlib.h>
#include "constants.h"
#include "geoCoord.h"
#include "h3Index.h"
#include "stackAlloc.h"
#include "test.h"

// Fixtures
Expand All @@ -31,7 +33,7 @@ BEGIN_TESTS(h3UniEdge);

TEST(h3IndexesAreNeighbors) {
H3Index sf = H3_EXPORT(geoToH3)(&sfGeo, 9);
H3Index* ring = calloc(H3_EXPORT(maxKringSize)(1), sizeof(H3Index));
STACK_ARRAY_CALLOC(H3Index, ring, H3_EXPORT(maxKringSize)(1));
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

H3_EXPORT(hexRing)(sf, 1, ring);

t_assert(H3_EXPORT(h3IndexesAreNeighbors)(sf, sf) == 0,
Expand All @@ -46,7 +48,7 @@ TEST(h3IndexesAreNeighbors) {
t_assert(neighbors == 6,
"got the expected number of neighbors from a k-ring of 1");

H3Index* largerRing = calloc(H3_EXPORT(maxKringSize)(2), sizeof(H3Index));
STACK_ARRAY_CALLOC(H3Index, largerRing, H3_EXPORT(maxKringSize)(2));
H3_EXPORT(hexRing)(sf, 2, largerRing);

neighbors = 0;
Expand All @@ -58,7 +60,6 @@ TEST(h3IndexesAreNeighbors) {
}
t_assert(neighbors == 0,
"got no neighbors, as expected, from a k-ring of 2");
free(largerRing);

H3Index sfBroken = sf;
H3_SET_MODE(sfBroken, H3_UNIEDGE_MODE);
Expand All @@ -71,15 +72,13 @@ TEST(h3IndexesAreNeighbors) {

t_assert(H3_EXPORT(h3IndexesAreNeighbors)(ring[2], ring[1]) == 1,
"hexagons in a ring are neighbors");
free(ring);
}

TEST(getH3UnidirectionalEdgeAndFriends) {
H3Index sf = H3_EXPORT(geoToH3)(&sfGeo, 9);
H3Index* ring = calloc(H3_EXPORT(maxKringSize)(1), sizeof(H3Index));
STACK_ARRAY_CALLOC(H3Index, ring, H3_EXPORT(maxKringSize)(1));
H3_EXPORT(hexRing)(sf, 1, ring);
H3Index sf2 = ring[0];
free(ring);

H3Index edge = H3_EXPORT(getH3UnidirectionalEdge)(sf, sf2);
t_assert(sf == H3_EXPORT(getOriginH3IndexFromUnidirectionalEdge)(edge),
Expand All @@ -95,10 +94,9 @@ TEST(getH3UnidirectionalEdgeAndFriends) {
t_assert(originDestination[1] == sf2,
"got the destination last in the pair request");

H3Index* largerRing = calloc(H3_EXPORT(maxKringSize)(2), sizeof(H3Index));
STACK_ARRAY_CALLOC(H3Index, largerRing, H3_EXPORT(maxKringSize)(2));
H3_EXPORT(hexRing)(sf, 2, largerRing);
H3Index sf3 = largerRing[0];
free(largerRing);

H3Index notEdge = H3_EXPORT(getH3UnidirectionalEdge)(sf, sf3);
t_assert(notEdge == 0, "Non-neighbors can't have edges");
Expand Down Expand Up @@ -135,10 +133,9 @@ TEST(getH3UnidirectionalEdgeFromPentagon) {

TEST(h3UnidirectionalEdgeIsValid) {
H3Index sf = H3_EXPORT(geoToH3)(&sfGeo, 9);
H3Index* ring = calloc(H3_EXPORT(maxKringSize)(1), sizeof(H3Index));
STACK_ARRAY_CALLOC(H3Index, ring, H3_EXPORT(maxKringSize)(1));
H3_EXPORT(hexRing)(sf, 1, ring);
H3Index sf2 = ring[0];
free(ring);

H3Index edge = H3_EXPORT(getH3UnidirectionalEdge)(sf, sf2);
t_assert(H3_EXPORT(h3UnidirectionalEdgeIsValid)(edge) == 1,
Expand Down Expand Up @@ -166,7 +163,7 @@ TEST(h3UnidirectionalEdgeIsValid) {

TEST(getH3UnidirectionalEdgesFromHexagon) {
H3Index sf = H3_EXPORT(geoToH3)(&sfGeo, 9);
H3Index* edges = calloc(6, sizeof(H3Index));
STACK_ARRAY_CALLOC(H3Index, edges, 6);
H3_EXPORT(getH3UnidirectionalEdgesFromHexagon)(sf, edges);

for (int i = 0; i < 6; i++) {
Expand All @@ -179,10 +176,11 @@ TEST(getH3UnidirectionalEdgesFromHexagon) {
edges[i]),
"destination is not origin");
}
free(edges);
}

TEST(getH3UnidirectionalEdgesFromPentagon) {
H3Index pentagon = 0x821c07fffffffff;
edges = calloc(6, sizeof(H3Index));
STACK_ARRAY_CALLOC(H3Index, edges, 6);
H3_EXPORT(getH3UnidirectionalEdgesFromHexagon)(pentagon, edges);

int missingEdgeCount = 0;
Expand All @@ -196,47 +194,118 @@ TEST(getH3UnidirectionalEdgesFromHexagon) {
pentagon ==
H3_EXPORT(getOriginH3IndexFromUnidirectionalEdge)(edges[i]),
"origin is correct");
t_assert(
sf != H3_EXPORT(getDestinationH3IndexFromUnidirectionalEdge)(
edges[i]),
"destination is not origin");
t_assert(pentagon !=
H3_EXPORT(getDestinationH3IndexFromUnidirectionalEdge)(
edges[i]),
"destination is not origin");
}
}
t_assert(missingEdgeCount == 1,
"Only one edge was deleted for the pentagon");
free(edges);
}

TEST(getH3UnidirectionalEdgeBoundary) {
H3Index sf = H3_EXPORT(geoToH3)(&sfGeo, 9);
H3Index* edges = calloc(6, sizeof(H3Index));
H3_EXPORT(getH3UnidirectionalEdgesFromHexagon)(sf, edges);

GeoBoundary gb;
for (int i = 0; i < 6; i++) {
H3_EXPORT(getH3UnidirectionalEdgeBoundary)(edges[i], &gb);
t_assert(gb.numVerts == 2, "Got the expected number of vertices back");
H3Index sf;
GeoBoundary boundary;
GeoBoundary edgeBoundary;
STACK_ARRAY_CALLOC(H3Index, edges, 6);

const int expectedVertices[][2] = {{3, 4}, {1, 2}, {2, 3},
{5, 0}, {4, 5}, {0, 1}};
Copy link
Contributor

Choose a reason for hiding this comment

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

const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will add.

Copy link
Contributor

Choose a reason for hiding this comment

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

Waiting on this when you have a chance.


for (int res = 0; res < 13; res++) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that the current approach fails at res > 12 because of the geo comparison precision.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding as a comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do.

sf = H3_EXPORT(geoToH3)(&sfGeo, res);
H3_EXPORT(h3ToGeoBoundary)(sf, &boundary);
H3_EXPORT(getH3UnidirectionalEdgesFromHexagon)(sf, edges);

for (int i = 0; i < 6; i++) {
H3_EXPORT(getH3UnidirectionalEdgeBoundary)(edges[i], &edgeBoundary);
t_assert(edgeBoundary.numVerts == 2,
"Got the expected number of vertices back");
for (int j = 0; j < edgeBoundary.numVerts; j++) {
t_assert(
geoAlmostEqual(&edgeBoundary.verts[j],
&boundary.verts[expectedVertices[i][j]]),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm too tired to figure this out at the moment. Looks fun :).

"Got expected vertex");
}
}
}
free(edges);
}

H3Index pentagon = 0x811c0ffffffffff;
edges = calloc(6, sizeof(H3Index));
H3_EXPORT(getH3UnidirectionalEdgesFromHexagon)(pentagon, edges);
TEST(getH3UnidirectionalEdgeBoundaryPentagonClassIII) {
H3Index pentagon;
GeoBoundary boundary;
GeoBoundary edgeBoundary;
STACK_ARRAY_CALLOC(H3Index, edges, 6);

const int expectedVertices[][3] = {{-1, -1, -1}, {2, 3, 4}, {4, 5, 6},
{8, 9, 0}, {6, 7, 8}, {0, 1, 2}};

for (int res = 1; res < 13; res += 2) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add comments for why we don't test all resolutions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added.

setH3Index(&pentagon, res, 24, 0);
H3_EXPORT(h3ToGeoBoundary)(pentagon, &boundary);
H3_EXPORT(getH3UnidirectionalEdgesFromHexagon)(pentagon, edges);

int missingEdgeCount = 0;
for (int i = 0; i < 6; i++) {
if (edges[i] == 0) {
missingEdgeCount++;
} else {
H3_EXPORT(getH3UnidirectionalEdgeBoundary)
(edges[i], &edgeBoundary);
t_assert(
edgeBoundary.numVerts == 3,
"Got the expected number of vertices back for a Class III "
"pentagon");
for (int j = 0; j < edgeBoundary.numVerts; j++) {
t_assert(
geoAlmostEqual(&edgeBoundary.verts[j],
&boundary.verts[expectedVertices[i][j]]),
"Got expected vertex");
}
}
}
t_assert(missingEdgeCount == 1,
"Only one edge was deleted for the pentagon");
}
}

int missingEdgeCount = 0;
for (int i = 0; i < 6; i++) {
if (edges[i] == 0) {
missingEdgeCount++;
} else {
H3_EXPORT(getH3UnidirectionalEdgeBoundary)(edges[i], &gb);
t_assert(gb.numVerts == 3,
"Got the expected number of vertices back for a Class III "
"pentagon");
TEST(getH3UnidirectionalEdgeBoundaryPentagonClassII) {
H3Index pentagon;
GeoBoundary boundary;
GeoBoundary edgeBoundary;
STACK_ARRAY_CALLOC(H3Index, edges, 6);

const int expectedVertices[][3] = {{-1, -1}, {1, 2}, {2, 3},
{4, 0}, {3, 4}, {0, 1}};

for (int res = 0; res < 12; res += 2) {
setH3Index(&pentagon, res, 24, 0);
H3_EXPORT(h3ToGeoBoundary)(pentagon, &boundary);
H3_EXPORT(getH3UnidirectionalEdgesFromHexagon)(pentagon, edges);

int missingEdgeCount = 0;
for (int i = 0; i < 6; i++) {
if (edges[i] == 0) {
missingEdgeCount++;
} else {
H3_EXPORT(getH3UnidirectionalEdgeBoundary)
(edges[i], &edgeBoundary);
t_assert(
edgeBoundary.numVerts == 2,
"Got the expected number of vertices back for a Class II "
"pentagon");
for (int j = 0; j < edgeBoundary.numVerts; j++) {
t_assert(
geoAlmostEqual(&edgeBoundary.verts[j],
&boundary.verts[expectedVertices[i][j]]),
"Got expected vertex");
}
}
}
t_assert(missingEdgeCount == 1,
"Only one edge was deleted for the pentagon");
}
t_assert(missingEdgeCount == 1,
"Only one edge was deleted for the pentagon");
free(edges);
}

END_TESTS();
39 changes: 34 additions & 5 deletions src/h3lib/lib/h3UniEdge.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/

#include <inttypes.h>
#include <stdbool.h>
#include "algos.h"
#include "constants.h"
#include "coordijk.h"
Expand Down Expand Up @@ -212,6 +213,22 @@ void H3_EXPORT(getH3UnidirectionalEdgesFromHexagon)(H3Index origin,
}
}

/**
* Whether the given coordinate has a matching vertex in the given geo boundary.
* @param vertex Coordinate to check
* @param boundary Geo boundary to look in
* @return Whether a match was found
*/
static bool _hasMatchingVertex(const GeoCoord* vertex,
Copy link
Contributor

Choose a reason for hiding this comment

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

Great!

const GeoBoundary* boundary) {
for (int i = 0; i < boundary->numVerts; i++) {
if (geoAlmostEqualThreshold(vertex, &boundary->verts[i], 0.000001)) {
return true;
}
}
return false;
}

/**
* Provides the coordinates defining the unidirectional edge.
* @param edge The unidirectional edge H3Index
Expand All @@ -221,6 +238,9 @@ void H3_EXPORT(getH3UnidirectionalEdgeBoundary)(H3Index edge, GeoBoundary* gb) {
// TODO: More efficient solution :)
GeoBoundary origin = {0};
GeoBoundary destination = {0};
GeoCoord postponedVertex = {0};
bool hasPostponedVertex = false;

H3_EXPORT(h3ToGeoBoundary)
(H3_EXPORT(getOriginH3IndexFromUnidirectionalEdge)(edge), &origin);
H3_EXPORT(h3ToGeoBoundary)
Expand All @@ -229,14 +249,23 @@ void H3_EXPORT(getH3UnidirectionalEdgeBoundary)(H3Index edge, GeoBoundary* gb) {

int k = 0;
for (int i = 0; i < origin.numVerts; i++) {
for (int j = 0; j < destination.numVerts; j++) {
if (geoAlmostEqualThreshold(&origin.verts[i], &destination.verts[j],
0.000001)) {
gb->verts[k].lat = origin.verts[i].lat;
gb->verts[k].lon = origin.verts[i].lon;
if (_hasMatchingVertex(&origin.verts[i], &destination)) {
// If we are on vertex 0, we need to handle the case where it's the
// end of the edge, not the beginning.
if (i == 0 &&
!_hasMatchingVertex(&origin.verts[i + 1], &destination)) {
postponedVertex = origin.verts[i];
hasPostponedVertex = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is zero a valid vertex? If not, you can just check that postponedVertex != 0 instead of maintaining a separate flag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

postponedVertex is a GeoCoord struct, and unfortunately, as @isaacbrodsky points out, it's valid to have all the fields in the struct initialized to 0. So I don't know of another way to check whether postponedVertex has been set or not, hence the flag. Open to suggestions.

} else {
gb->verts[k] = origin.verts[i];
k++;
}
}
}
// If we postponed adding the last vertex, add it now
if (hasPostponedVertex) {
gb->verts[k] = postponedVertex;
k++;
}
gb->numVerts = k;
}