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

Add isValidVertex #420

Merged
merged 3 commits into from
Jan 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ The public API of this library consists of the functions declared in file
- `cellToVertex(cell, vertexNum)`
- `cellToVertexes(cell, out)`
- `vertexToPoint(vertex, out)`
- `isValidVertex(vertex)`
- closed-form implementation of `numHexagons`

### Breaking changes
Expand Down
53 changes: 53 additions & 0 deletions src/apps/testapps/testVertex.c
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,57 @@ SUITE(Vertex) {
t_assert(directionForVertexNum(pentagon, 5) == INVALID_DIGIT,
"invalid pent vertex should return invalid direction");
}

TEST(isValidVertex_hex) {
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 a test for a specific valid vertex, to validate the right mode number is being used.

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.

H3Index origin = 0x823d6ffffffffff;
H3Index vert = 0x2222597fffffffff;

t_assert(H3_EXPORT(isValidVertex)(vert), "known vertex is valid");

for (int i = 0; i < NUM_HEX_VERTS; i++) {
vert = H3_EXPORT(cellToVertex)(origin, i);
t_assert(H3_EXPORT(isValidVertex)(vert), "vertex is valid");
}
}

TEST(isValidVertex_badOwner) {
H3Index origin = 0x823d6ffffffffff;
int vertexNum = 0;
H3Index vert = H3_EXPORT(cellToVertex)(origin, vertexNum);

// Assert that origin does not own the vertex
H3Index owner = vert;
H3_SET_MODE(owner, H3_HEXAGON_MODE);
H3_SET_RESERVED_BITS(owner, 0);

t_assert(origin != owner, "origin does not own the canonical vertex");

H3Index nonCanonicalVertex = origin;
H3_SET_MODE(nonCanonicalVertex, H3_VERTEX_MODE);
H3_SET_RESERVED_BITS(nonCanonicalVertex, vertexNum);

t_assert(H3_EXPORT(isValidVertex)(nonCanonicalVertex) == 0,
"vertex with incorrect owner is not valid");
}

TEST(isValidVertex_badVerts) {
H3Index origin = 0x823d6ffffffffff;
t_assert(H3_EXPORT(isValidVertex)(origin) == 0, "cell is not valid");

H3Index fakeEdge = origin;
H3_SET_MODE(fakeEdge, H3_UNIEDGE_MODE);
t_assert(H3_EXPORT(isValidVertex)(fakeEdge) == 0,
"edge mode is not valid");

H3Index vert = H3_EXPORT(cellToVertex)(origin, 0);
H3_SET_RESERVED_BITS(vert, 6);
t_assert(H3_EXPORT(isValidVertex)(vert) == 0,
"invalid vertexNum is not valid");

H3Index pentagon = 0x823007fffffffff;
H3Index vert2 = H3_EXPORT(cellToVertex)(pentagon, 0);
H3_SET_RESERVED_BITS(vert2, 5);
t_assert(H3_EXPORT(isValidVertex)(vert2) == 0,
"invalid pentagon vertexNum is not valid");
}
}
19 changes: 19 additions & 0 deletions src/apps/testapps/testVertexExhaustive.c
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,17 @@ static void cellToVertex_uniqueness_assertions(H3Index h3) {
}
}

static void cellToVertex_validity_assertions(H3Index h3) {
H3Index verts[NUM_HEX_VERTS] = {0};
H3_EXPORT(cellToVertexes)(h3, verts);

for (int i = 0; i < NUM_HEX_VERTS - 1; i++) {
if (verts[i] != H3_NULL) {
t_assert(H3_EXPORT(isValidVertex(verts[i])), "vertex is valid");
}
}
}

static void cellToVertex_neighbor_assertions(H3Index h3) {
H3Index neighbors[7] = {0};
H3Index originVerts[NUM_HEX_VERTS] = {0};
Expand Down Expand Up @@ -130,4 +141,12 @@ SUITE(Vertex) {
iterateAllIndexesAtRes(3, cellToVertex_uniqueness_assertions);
iterateAllIndexesAtRes(4, cellToVertex_uniqueness_assertions);
}

TEST(cellToVertex_validity) {
iterateAllIndexesAtRes(0, cellToVertex_validity_assertions);
iterateAllIndexesAtRes(1, cellToVertex_validity_assertions);
iterateAllIndexesAtRes(2, cellToVertex_validity_assertions);
iterateAllIndexesAtRes(3, cellToVertex_validity_assertions);
iterateAllIndexesAtRes(4, cellToVertex_validity_assertions);
}
}
8 changes: 8 additions & 0 deletions src/h3lib/include/h3api.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,14 @@ DECLSPEC void H3_EXPORT(cellToVertexes)(H3Index origin, H3Index *vertexes);
DECLSPEC void H3_EXPORT(vertexToPoint)(H3Index vertex, GeoCoord *coord);
/** @} */

/** @defgroup isValidVertex isValidVertex
* Functions for isValidVertex
* @{
*/
/** @brief Whether the input is a valid H3 vertex */
DECLSPEC int H3_EXPORT(isValidVertex)(H3Index vertex);
/** @} */

/** @defgroup h3Distance h3Distance
* Functions for h3Distance
* @{
Expand Down
28 changes: 27 additions & 1 deletion src/h3lib/lib/vertex.c
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ H3Index H3_EXPORT(cellToVertex)(H3Index cell, int vertexNum) {
// Note that vertex - 1 is the right side, as vertex numbers are CCW
Direction right = directionForVertexNum(
cell, (vertexNum - 1 + cellNumVerts) % cellNumVerts);
// This case should be unreachable; invalid verts fail the left side first
// This case should be unreachable; invalid verts fail the left side first
if (right == INVALID_DIGIT) return H3_NULL; // LCOV_EXCL_LINE
int lRotations = 0;
H3Index rightNeighbor = h3NeighborRotations(cell, right, &lRotations);
Expand Down Expand Up @@ -269,3 +269,29 @@ void H3_EXPORT(vertexToPoint)(H3Index vertex, GeoCoord* coord) {
// Copy from boundary to output coord
*coord = gb.verts[0];
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@ajfriend ajfriend Jan 12, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

if (H3_GET_MODE(vertex) != H3_VERTEX_MODE) {
return 0;
}

int vertexNum = H3_GET_RESERVED_BITS(vertex);
H3Index owner = vertex;
Copy link
Contributor

Choose a reason for hiding this comment

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

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)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

H3_SET_MODE(owner, H3_HEXAGON_MODE);
H3_SET_RESERVED_BITS(owner, 0);

if (!H3_EXPORT(h3IsValid)(owner)) {
return 0;
}

// The easiest way to ensure that the owner + vertex number is valid,
// and that the vertex is canonical, is to recreate and compare.
H3Index canonical = H3_EXPORT(cellToVertex)(owner, vertexNum);

return vertex == canonical ? 1 : 0;
}