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

(Undirected) Edge indexing mode #654

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

isaacbrodsky
Copy link
Collaborator

@isaacbrodsky isaacbrodsky commented Aug 24, 2022

@coveralls
Copy link

coveralls commented Aug 24, 2022

Coverage Status

Coverage: 98.766% (+0.1%) from 98.657% when pulling 7aad06b on isaacbrodsky:edge into 5c91149 on uber:master.

src/apps/testapps/testEdge.c Outdated Show resolved Hide resolved
t_assert(cells[0] < cells[1],
"Directed edge cells are in normalized order");

t_assert(H3_EXPORT(edgeToCells)(0, cells) == E_UNDIR_EDGE_INVALID,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I'd move the failure cases to one or more separate tests for legibility

src/apps/testapps/testEdge.c Show resolved Hide resolved
src/h3lib/include/h3api.h.in Outdated Show resolved Hide resolved
src/h3lib/include/h3api.h.in Outdated Show resolved Hide resolved
src/h3lib/lib/edge.c Outdated Show resolved Hide resolved
H3Index edge = origin;
H3_SET_MODE(edge, H3_EDGE_MODE);
H3_SET_RESERVED_BITS(edge, i + 1);
H3Error normalizeError = normalizeEdge(edge, &edges[i]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: The same perf concerns apply here. Every edge here is potentially non-normalized, so we could end up running directionForNeighbor 6 times. I think it might be more efficient to take gridDisk(origin, 1) and then loop through the neighbors running cellsToEdge for each. You could even use edges for the storage and then replace each neighbor with the origin-neighbor edge, that way you don't have to allocate any new memory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

edges can't be used for storage because gridDisk needs an array of length 7. gridRing can't be used because there is no "safe" version. We would need a different function for getting just the immediate neighbors of a cell.

src/h3lib/lib/edge.c Outdated Show resolved Hide resolved
*
* @return length in radians
*/
H3Error H3_EXPORT(edgeLengthRads)(H3Index edge, double *length) {
CellBoundary cb;

if (H3_EXPORT(isValidEdge)(edge)) {
// TODO: This could potentially generate a E_DIREDGE_INVALID error
// which would be confusing.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We just checked isValidEdge, so this concern is academic, right?

website/docs/core-library/h3indexing.md Outdated Show resolved Hide resolved
@isaacbrodsky isaacbrodsky marked this pull request as ready for review December 28, 2022 17:15
@isaacbrodsky
Copy link
Collaborator Author

This PR is now ready for review

Copy link
Collaborator

@nrabinowitz nrabinowitz left a comment

Choose a reason for hiding this comment

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

Looks good!

"One of the cells is the destination");
t_assert(sf != sf2, "Sanity check for cells not being the same");
t_assert(cells[0] < cells[1],
"Directed edge cells are in normalized order");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Not sure it makes sense to test this, as from an API perspective the order of these two cells is undefined. Also the comment doesn't make sense (should be "undirected", but then the order doesn't matter)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is relying on an implementation detail that the first cell returned is the "origin". We could choose to make that a defined part of the API.

I'll fix the comment.

h3NeighborRotations(owner, dir, &rotations, &destination));
t_assert(owner == sf || destination == sf,
"original cell is owner or neighbor");
t_assert(owner < destination, "owning cell sorts first");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: As above, I don't think this is worth a test, as it's an implementation detail

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Which cell is the owner and which is the non-owner seems to be part of the contract of undirected edge mode, though. That seems to need testing to ensure it is always adhered to. It can't really be changed after the fact, can it?

}
}

SUITE(edge) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for the exhaustive tests here, this is great

website/docs/library/index/h3indexing.md Outdated Show resolved Hide resolved
| edgeToCells | [fuzzerEdge](./fuzzerEdge.c)
| cellToEdges | [fuzzerEdge](./fuzzerEdge.c)
| edgeToBoundary | [fuzzerEdge](./fuzzerEdge.c)
| directedEdgeToEdge | [fuzzerEdge](./fuzzerEdge.c)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit (and here just because it provides a nice summary): thoughts on the value of a edgeToDirectedEdges function?

slug: /library/index/edge
---

An H3 Edge index (mode 3) represents a single undirected edge between two cells. One of the two cells is arbitrarily picked as an "owner" or "origin" cell, which is used to calculate the canonical index of the edge. The components of the H3 Edge index are packed into a 64-bit integer in order, highest bit first, as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

In the abstraction, no cell is the "owner" of the edge because it is just an unordered pair of adjacent cells. In the internal implementation, we choose an "owner", but I wouldn't say it is arbitrary. The "owner" is unambiguously the smaller of the two cells in the "canonical ordering".

Copy link
Contributor

Choose a reason for hiding this comment

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

Thought: There should be no concept of "owner" or "origin" in the API for undirected edges, but it probably does make sense to talk about in terms of the implementation details. I'm not sure how best to allocate those levels of specificity in the docs, tho :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the phrase "arbitrary" and explained the exact rule.

I think we need to explain how the canonical index is arrived at and why it represents the unordered edge. I would put further explanation of how to think about the mode and how to use it before we introduce the technical details of how it's encoded. Right now we don't have that content on this page. For the API, we don't commit to ordering for the most part, although I believe I do rely on that in some of the tests.

@ajfriend
Copy link
Contributor

I think it makes sense to keep the API agnostic as to the ordering of the cells, but the internal representation should always have them in the "canonical ordering". Does that seem like the right approach?

Comment on lines +62 to +73
H3Error H3_EXPORT(cellsToEdge)(H3Index cell1, H3Index cell2, H3Index *out) {
bool cell1IsOrigin = cell1 < cell2;
H3Index origin = cell1IsOrigin ? cell1 : cell2;
H3Index dest = cell1IsOrigin ? cell2 : cell1;
H3Error edgeErr = H3_EXPORT(cellsToDirectedEdge)(origin, dest, out);
if (!edgeErr) {
H3_SET_MODE(*out, H3_EDGE_MODE);
return edgeErr;
} else {
return edgeErr;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The boolean check based on the numeric value of the indexes will make sense most of the time, but will probably cause unexpected weirdness along base cell boundaries. I don't have a better idea here, though.

Also, perhaps cellsToDirectedEdge should be macro-ified and re-used in both, without needing to set the mode bit twice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you describe the weirdness you are anticipating? I am not sure what would be odd. I expect the cell with the lower base cell number would always be chosen along the boundary with another base cell.

Copy link
Collaborator

@dfellis dfellis Jan 17, 2023

Choose a reason for hiding this comment

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

So here's a crude diagram showing the underlying directed edges for the 12 undirected edges amongst a set of "sibling" cells.

undirected_edge_inner_directions

There's an asymmetry in the number of edges that are the origin or destination for any given cell, with the center cells having 6 origins and no destinations, the "1", "2", and "4" siblings after that having 2 origins and 1 destination, and the "3", "5", and "6" siblings having no origins and 3 destinations.

That in itself is a little odd, but not terrible in that it repeats. Cells that aren't "siblings" will be harder to predict for users, but make sense in a fashion as long as they have the same base cell, as the orientation of the edge will be determined by the tree structure of their parent cells' border, or grandparent cells' border, etc.

But once you get to the base cells, they are simply hardwired on each icosahedron face and this pattern breaks and there's no spatial rhyme or reason to the selection.

When this was proposed I was assuming we'd do something like the local IJ coordinate system and choose the cell with the smaller absolute value of the magnitude of the IJ vector or something, and thought the holdup was just figuring out a performant way to do it.

This would have some spatial relationship, in that the vectors would tend to point "out from" the center of each icosahedron face and point into each pentagon, and hexagons along the border of the icosahedron face would, I believe, tend to point east in roughly the north pole region, west in the south pole region, and alternate east and west in the equatorial "band" of triangle faces, where the vector sum of all of these edges should be zero.

I had mentioned the "smallest integer index wins" approach when I did the first version of directed edges, but as a "that's terrible" deprecating statement that I couldn't figure out a better solution at the time.

return 0;
}

return H3_EXPORT(isValidCell)(cells[0]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be:

Suggested change
return H3_EXPORT(isValidCell)(cells[0]);
return H3_EXPORT(isValidCell)(cells[0]) && H3_EXPORT(isValidCell)(cells[1]);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My assumption is that if the owning cell is valid, and edgeToCells did not error, that cells[1] must also be valid. Maybe we should add an ALWAYS check here that asserts that, in the interests of completeness.

* @param cell2 A neighboring H3 hexagon index
* @param out Output: the edge H3Index
*/
H3Error H3_EXPORT(cellsToEdge)(H3Index cell1, H3Index cell2, H3Index *out) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a blocker on this PR, but there may be a cellsToEdges function that can take any arbitrary indexes and return an array of edges between them, similar to gridPathCells?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that's intended to be named gridPathEdges (which is why gridPathCells has Cells in the name)

Copy link
Collaborator

@dfellis dfellis left a comment

Choose a reason for hiding this comment

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

No blocking comments.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ isaacbrodsky
❌ Isaac Brodsky


Isaac Brodsky seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants