Fix for edge directions in getH3UnidirectionalEdgeBoundary #78

nrabinowitz · 2018-06-12T20:50:42Z

Fixes #75. This isn't the most elegant fix, but it does get the vertices into the correct order.

Longer term, I think the best option here is to do what we do in h3SetToVertexGraph, getting the edges and then walking them. But at the moment this was a bit too clunky to implement; it might be easier after a refactor of the VertexGraph structure.

nrabinowitz · 2018-06-12T20:51:11Z

src/apps/testapps/testH3UniEdge.c

+    int expectedVertices[][2] = {{3, 4}, {1, 2}, {2, 3},
+                                 {5, 0}, {4, 5}, {0, 1}};
+
+    for (int res = 0; res < 13; res++) {


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

Might be worth adding as a comment.

coveralls · 2018-06-12T20:53:31Z

Coverage remained the same at 100.0% when pulling 2d93522 on nrabinowitz:edge-boundary into 1d3360f on uber:master.

isaacbrodsky · 2018-06-12T21:05:33Z

src/h3lib/lib/h3UniEdge.c

@@ -212,6 +213,15 @@ void H3_EXPORT(getH3UnidirectionalEdgesFromHexagon)(H3Index origin,
    }
 }

+bool _hasMatchingVertex(GeoCoord* vertex, GeoBoundary* boundary) {


These arguments should be const.

Will update.

isaacbrodsky · 2018-06-12T21:06:19Z

src/h3lib/lib/h3UniEdge.c

+            // end of the edge, not the beginning.
+            // TODO: This is fine for this limited case, but it might be
+            // better/cleaner to use a VertexGraph for this
+            printf("%d, %d\n", i,


Please delete debugging statement

Oh, sorry. Will delete.

isaacbrodsky · 2018-06-12T21:07:39Z

src/h3lib/lib/h3UniEdge.c

                k++;
            }
        }
    }
+    // If we postponed adding the last vertex, add it now
+    if (postponedVertex.lat && postponedVertex.lon) {


This doesn't look safe, since (0,0) is a valid set of coordinates.

True. Is there a better option here? The only thing I can think of is to add an additional var like hasPostponedVertex, which is pretty ugly.

isaachier

A few comments. Still struggling with the concepts here, but I'll read more of https://uber.github.io/h3 when I am less tired.

isaachier · 2018-06-12T23:11:59Z

src/apps/testapps/testH3UniEdge.c

+    int expectedVertices[][2] = {{3, 4}, {1, 2}, {2, 3},
+                                 {5, 0}, {4, 5}, {0, 1}};
+
+    for (int res = 0; res < 13; res++) {


Might be worth adding as a comment.

isaachier · 2018-06-12T23:14:36Z

src/apps/testapps/testH3UniEdge.c

-    H3Index sf = H3_EXPORT(geoToH3)(&sfGeo, 9);
+    H3Index sf;
+    GeoBoundary boundary;
+    GeoBoundary edgeBoundary;
    H3Index* edges = calloc(6, sizeof(H3Index));


Somewhat outside of your changeset, but why is calloc here instead of STACK_ARRAY_CALLOC (https://github.com/uber/h3/blob/master/src/h3lib/include/stackAlloc.h#L29)?

Mostly likely because this was written before we introduced that. Will update.

isaachier · 2018-06-12T23:18:20Z

src/apps/testapps/testH3UniEdge.c

-        H3_EXPORT(getH3UnidirectionalEdgeBoundary)(edges[i], &gb);
-        t_assert(gb.numVerts == 2, "Got the expected number of vertices back");
+    int expectedVertices[][2] = {{3, 4}, {1, 2}, {2, 3},
+                                 {5, 0}, {4, 5}, {0, 1}};


Waiting on this when you have a chance.

isaachier · 2018-06-12T23:19:31Z

src/apps/testapps/testH3UniEdge.c

-            t_assert(gb.numVerts == 3,
-                     "Got the expected number of vertices back for a Class III "
-                     "pentagon");
+    int expectedVertices[][3] = {{-1, -1, -1}, {2, 3, 4}, {4, 5, 6},


Added, that's what I get for trying to multitask.

isaachier · 2018-06-12T23:19:45Z

src/apps/testapps/testH3UniEdge.c

+    H3Index pentagon;
+    GeoBoundary boundary;
+    GeoBoundary edgeBoundary;
+    H3Index* edges = calloc(6, sizeof(H3Index));


Same stack allocation question.

isaachier · 2018-06-12T23:30:48Z

src/apps/testapps/testH3UniEdge.c

+            for (int j = 0; j < edgeBoundary.numVerts; j++) {
+                t_assert(
+                    geoAlmostEqual(&edgeBoundary.verts[j],
+                                   &boundary.verts[expectedVertices[i][j]]),


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

isaachier · 2018-06-12T23:41:06Z

src/h3lib/lib/h3UniEdge.c

@@ -212,6 +213,15 @@ void H3_EXPORT(getH3UnidirectionalEdgesFromHexagon)(H3Index origin,
    }
 }

+bool _hasMatchingVertex(const GeoCoord* vertex, const GeoBoundary* boundary) {


Will update.

isaachier · 2018-06-12T23:42:56Z

src/h3lib/lib/h3UniEdge.c

+            // If we are on vertex 0, we need to handle the case where it's the
+            // end of the edge, not the beginning.
+            // TODO: This is fine for this limited case, but it might be
+            // better/cleaner to use a VertexGraph for this


You are better off avoiding allocations involved in creating a VertexGraph and hacking a solution with only the given inputs.

Yeah, probably true. It just irks me slightly that this is basically the same thing we're doing in h3SetToVertexGraph but we're doing it totally differently :/. Also bothers me that this is O(n^2), even though n is guaranteed to be <= 10 and is usually 6. I'll drop the comment though, I doubt there'd be any benefit.

isaachier · 2018-06-12T23:44:04Z

src/h3lib/lib/h3UniEdge.c

+            if (i == 0 &&
+                !_hasMatchingVertex(&origin.verts[i + 1], &destination)) {
+                postponedVertex = origin.verts[i];
+                hasPostponedVertex = true;


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

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.

isaachier · 2018-06-12T23:44:52Z

src/h3lib/lib/h3UniEdge.c

    gb->numVerts = k;
-}
+}


Deleted newline from end of file. Put a little marker here to show me.

Hm. I'm surprised clang-format doesn't have some sort of opinion on that. Will fix.

isaachier · 2018-06-13T01:02:10Z

src/apps/testapps/testH3UniEdge.c

@@ -32,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));


isaachier · 2018-06-13T01:03:13Z

src/h3lib/lib/h3UniEdge.c

+ * @param  boundary Geo boundary to look in
+ * @return          Whether a match was found
+ */
+static bool _hasMatchingVertex(const GeoCoord* vertex,


isaachier · 2018-06-13T01:03:38Z

src/apps/testapps/testH3UniEdge.c

-        H3_EXPORT(getH3UnidirectionalEdgeBoundary)(edges[i], &gb);
-        t_assert(gb.numVerts == 2, "Got the expected number of vertices back");
+    int expectedVertices[][2] = {{3, 4}, {1, 2}, {2, 3},
+                                 {5, 0}, {4, 5}, {0, 1}};


Waiting on this when you have a chance.

isaacbrodsky · 2018-06-13T18:53:38Z

src/apps/testapps/testH3UniEdge.c

+    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) {


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

isaachier · 2018-06-14T21:37:01Z

It seems Travis CI wants you to format the code.

isaachier

Approving changes, but please reformat.

nrabinowitz · 2018-06-14T21:39:39Z

Yeah, having more trouble with this than expected due to laptop theft :(. Should update shortly.

CLAassistant · 2018-06-14T21:54:30Z

All committers have signed the CLA.

Fix for edge directions in getH3UnidirectionalEdgeBoundary

Fix for edge directions in getH3UnidirectionalEdgeBoundary

a25f18c

nrabinowitz requested review from dfellis and isaacbrodsky June 12, 2018 20:50

nrabinowitz commented Jun 12, 2018

View reviewed changes

isaacbrodsky reviewed Jun 12, 2018

View reviewed changes

isaacbrodsky requested a review from isaachier June 12, 2018 21:19

Code review updates

622f617

isaachier reviewed Jun 12, 2018

View reviewed changes

Comment updates, static and const, use STACK_ARRAY_ALLOC

ee30ce6

isaachier reviewed Jun 13, 2018

View reviewed changes

Add const

e5b130e

isaacbrodsky reviewed Jun 13, 2018

View reviewed changes

Add requested comments about resolution

e7001d9

isaachier approved these changes Jun 14, 2018

View reviewed changes

make format

2d93522

nrabinowitz force-pushed the edge-boundary branch from be7d923 to 2d93522 Compare June 14, 2018 21:55

nrabinowitz merged commit efafcd2 into uber:master Jun 14, 2018

isaacbrodsky mentioned this pull request Jul 23, 2018

Bump version to 3.0.8 uber/h3-js#10

Merged

mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022

Merge pull request uber#78 from nrabinowitz/edge-boundary

74ca05d

Fix for edge directions in getH3UnidirectionalEdgeBoundary

Fix for edge directions in getH3UnidirectionalEdgeBoundary #78

Fix for edge directions in getH3UnidirectionalEdgeBoundary #78

Conversation

nrabinowitz commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier commented Jun 14, 2018

isaachier left a comment

Choose a reason for hiding this comment

nrabinowitz commented Jun 14, 2018

CLAassistant commented Jun 14, 2018 • edited Loading

coveralls commented Jun 12, 2018 •

edited

Loading

CLAassistant commented Jun 14, 2018 •

edited

Loading