Add isValidVertex #420

nrabinowitz · 2021-01-09T16:59:07Z

Adds the isValidVertex function, with tests.

coveralls · 2021-01-09T17:02:11Z

Coverage decreased (-0.06%) to 99.183% when pulling 6690c22 on nrabinowitz:is-valid-vertex into 5988d7c on uber:master.

ajfriend · 2021-01-10T03:22:52Z

src/h3lib/lib/vertex.c

+    }
+
+    int vertexNum = H3_GET_RESERVED_BITS(vertex);
+    H3Index owner = vertex;


Do we need the extra variable owner here? Could you just work with vertex and avoid the copy? If just for naming purposes, maybe change the input parameter name to index (which is maybe actually more correct, since it is perfectly valid to input a non-vertex index into this function)?

That's a good question - I guess it should be fine to modify vertex, though I generally think it's confusing to modify arguments in-place. Will update.

I'm actually going to recommend the opposite tack. Getting the owner H3Index is vital because I think this function should confirm that it can reproduce the original vertex value in here.

That will make this function slower, but since you'd only be validating the vertex if you're not sure about it, and since there are non-canonical representations that could be reached through some bit manipulation, it feels like the only time you'd be calling isVertexValid would be when you aren't getting them generated by the H3 library itself.

Oh huh. I hadn't even thought of it, but yeah, the best way to check whether we have the canonical representation is to run this through cellToVertex again and compare output. Easier than what I was thinking, where I'd have to get neighbors again. Good idea.

ajfriend · 2021-01-10T03:28:47Z

src/h3lib/lib/vertex.c

+ * @param  vertex H3 index possibly describing a vertex
+ * @return        Whether the input is valid
+ */
+int H3_EXPORT(isValidVertex)(H3Index vertex) {


Do vertices have a unique representation? That is, for a given vertex, is it only allowed a single cell "owner"?

It doesn't look like this code checks if the vertex has the correct owner.

My assumption is this function will assure the vertex is in the canonical form, if we want to support non-canonical vertexes, perhaps we need canonicalizeVertex/isCanonicalVertex?

Hm. That's a good point, though there's no way at present that you can create a non-canonical vertex, so I'm leery about adding public functions addressing a practically unreachable issue. If we wanted this check, I think I'd need to implement vertexToCells first, and then check that the owner had the lowest numerical index.

I don't have a clear picture for the validation functions on whether we should prioritize speed (because you might need to validate many indexes at once) or exhaustiveness, which in this case would mean an expensive check for an unlikely case.

So I guess this brings up two questions:

Is there a use for a non-canonical vertex (i.e. an index indicating a cell and vertex number)? This would be easy to produce, but I'm not sure it has any value, and it would muddy the waters about whether all vertexes are canonical.

If not, is it worth an expensive check in this function for a case that at present would need to be hand-crafted?

I think we definitely want uniqueness for the indices (and thus not allow "non-canonical" vertices), and if we have a function called isValidVertex it probably should be exhaustive and strictly correct. I think speed is secondary to correctness, and is something we could potentially improve in the future.

Updated to check validity by recreating the valid vertex and comparing, plus added a test for the incorrect-owner case. Recreating the vertex also covers the invalid vertex number case, so this simplified things nicely.

ajfriend · 2021-01-10T03:38:44Z

I probably should have figured this out with the previous PR, but how exactly do vertices work? Is it the case that some cells are allowed to "own" vertices, and some are not?

If so, would it be possible to provide a function is_allowed_to_own_vertices(H3Index cell)?

If that isn't the case, however, does that mean vertices are not uniquely represented? For example, for a given vertex, could be vertexNum=2 for one cell, but vertexNum=5 for a different incident cell?

ajfriend · 2021-01-10T03:51:18Z

Adding this for consideration: #421

isaacbrodsky · 2021-01-10T17:43:04Z

src/apps/testapps/testVertex.c

@@ -85,4 +85,35 @@ SUITE(Vertex) {
        t_assert(directionForVertexNum(pentagon, 5) == INVALID_DIGIT,
                 "invalid pent vertex should return invalid direction");
    }
+
+    TEST(isValidVertex_hex) {


Please add a test for a specific valid vertex, to validate the right mode number is being used.

nrabinowitz · 2021-01-11T17:27:23Z

I probably should have figured this out with the previous PR, but how exactly do vertices work? Is it the case that some cells are allowed to "own" vertices, and some are not?

See https://github.com/uber/h3/blob/master/dev-docs/RFCs/v4.0.0/vertex-mode-rfc.md - the basic idea is, you take the three cells sharing the vertex, and assign the lowest numeric index as the canonical owner. Ownership is an implementation detail, and shouldn't be exposed in the public API I think.

If so, would it be possible to provide a function is_allowed_to_own_vertices(H3Index cell)?

Nope, because a) not possible (a cell could own some vertices and not others), and b) not public info.

If that isn't the case, however, does that mean vertices are not uniquely represented? For example, for a given vertex, could be vertexNum=2 for one cell, but vertexNum=5 for a different incident cell?

See above - we just pick one cell and say that's the owner. What you're describing here is a non-canonical vertex, which I haven't implemented and don't see much use for, though let me know if you think there's some value.

isaacbrodsky · 2021-01-15T00:19:30Z

src/h3lib/lib/vertex.c

+        return 0;
+    }
+
+    return H3_EXPORT(h3IsValid)(owner);


May want to check that the owner is valid first?

Hm. I guess it depends whether h3IsValid is faster - but it also would avoid handing invalid indexes to cellToVertex, so I think that makes sense.

Add isValidVertex

3f91163

nrabinowitz requested review from dfellis, ajfriend and isaacbrodsky January 9, 2021 16:59

ajfriend reviewed Jan 10, 2021

View reviewed changes

isaacbrodsky reviewed Jan 10, 2021

View reviewed changes

Ensure non-canonical vertexes are not valid

84926df

dfellis approved these changes Jan 14, 2021

View reviewed changes

isaacbrodsky approved these changes Jan 15, 2021

View reviewed changes

Check h3IsValid before canonical vertex

6690c22

nrabinowitz merged commit dd0b1e5 into uber:master Jan 15, 2021

nrabinowitz deleted the is-valid-vertex branch January 15, 2021 01:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add isValidVertex #420

Add isValidVertex #420

nrabinowitz commented Jan 9, 2021

coveralls commented Jan 9, 2021 •

edited

Loading

ajfriend Jan 10, 2021

nrabinowitz Jan 11, 2021

dfellis Jan 14, 2021

nrabinowitz Jan 14, 2021

ajfriend Jan 10, 2021

isaacbrodsky Jan 10, 2021

nrabinowitz Jan 11, 2021

ajfriend Jan 12, 2021 •

edited

Loading

nrabinowitz Jan 14, 2021

ajfriend commented Jan 10, 2021

ajfriend commented Jan 10, 2021

isaacbrodsky Jan 10, 2021

nrabinowitz Jan 11, 2021

nrabinowitz commented Jan 11, 2021

isaacbrodsky Jan 15, 2021

nrabinowitz Jan 15, 2021

Add isValidVertex #420

Add isValidVertex #420

Conversation

nrabinowitz commented Jan 9, 2021

coveralls commented Jan 9, 2021 • 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

Choose a reason for hiding this comment

ajfriend Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajfriend commented Jan 10, 2021

ajfriend commented Jan 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrabinowitz commented Jan 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 9, 2021 •

edited

Loading

ajfriend Jan 12, 2021 •

edited

Loading