(Undirected) Edge indexing mode #654

isaacbrodsky · 2022-08-24T16:14:55Z

coveralls · 2022-08-24T19:37:04Z

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

src/apps/testapps/testEdge.c

nrabinowitz · 2022-09-08T18:27:51Z

src/apps/testapps/testEdge.c

+        t_assert(cells[0] < cells[1],
+                 "Directed edge cells are in normalized order");
+
+        t_assert(H3_EXPORT(edgeToCells)(0, cells) == E_UNDIR_EDGE_INVALID,


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

src/apps/testapps/testEdge.c

src/h3lib/include/h3api.h.in

src/h3lib/lib/edge.c

nrabinowitz · 2022-09-08T19:32:18Z

src/h3lib/lib/edge.c

+            H3Index edge = origin;
+            H3_SET_MODE(edge, H3_EDGE_MODE);
+            H3_SET_RESERVED_BITS(edge, i + 1);
+            H3Error normalizeError = normalizeEdge(edge, &edges[i]);


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.

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

nrabinowitz · 2022-09-08T19:34:39Z

src/h3lib/lib/latLng.c

 *
 * @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.


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

website/docs/core-library/h3indexing.md

isaacbrodsky · 2022-12-29T00:57:44Z

This PR is now ready for review

nrabinowitz

Looks good!

nrabinowitz · 2023-01-05T18:10:54Z

src/apps/testapps/testEdge.c

+                 "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");


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)

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.

nrabinowitz · 2023-01-05T18:15:01Z

src/apps/testapps/testEdge.c

+                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");


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

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?

nrabinowitz · 2023-01-05T18:23:56Z

src/apps/testapps/testEdgeExhaustive.c

+    }
+}
+
+SUITE(edge) {


+1 for the exhaustive tests here, this is great

website/docs/library/index/h3indexing.md

Co-authored-by: Nick Rabinowitz <[email protected]>

ajfriend · 2023-01-17T04:59:22Z

src/apps/fuzzers/README.md

+| edgeToCells | [fuzzerEdge](./fuzzerEdge.c)
+| cellToEdges | [fuzzerEdge](./fuzzerEdge.c)
+| edgeToBoundary | [fuzzerEdge](./fuzzerEdge.c)
+| directedEdgeToEdge | [fuzzerEdge](./fuzzerEdge.c)


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

ajfriend · 2023-01-17T05:05:20Z

website/docs/library/index/edge.md

+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:


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".

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 :/

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 · 2023-01-17T05:10:25Z

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?

dfellis · 2023-01-17T05:49:58Z

src/h3lib/lib/edge.c

+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;
+    }
+}


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?

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.

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

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.

dfellis · 2023-01-17T05:51:21Z

src/h3lib/lib/edge.c

+        return 0;
+    }
+
+    return H3_EXPORT(isValidCell)(cells[0]);


Should this be:

Suggested change

return H3_EXPORT(isValidCell)(cells[0]);

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

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.

dfellis · 2023-01-17T05:53:45Z

src/h3lib/lib/edge.c

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


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?

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

dfellis

No blocking comments.

CLAassistant · 2023-02-16T07:46:14Z

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.}

Isaac Brodsky added 3 commits August 22, 2022 14:51

(nondirected) edge mode

9322d5f

update tests

09a1603

Merge branch 'master' into edge

6b3e3ab

isaacbrodsky requested review from nrabinowitz, dfellis and ajfriend August 24, 2022 16:14

Isaac Brodsky added 2 commits August 24, 2022 09:15

changelog

7a58fb7

fix missing H3_EXPORT macro

12a5f67

nrabinowitz reviewed Sep 8, 2022

View reviewed changes

isaacbrodsky and others added 7 commits September 9, 2022 08:28

Standardize undirected edge term per review

1dec0fc

Merge branch 'master' into edge

f9fa92c

Merge branch 'master' into edge

d023233

Merge branch 'master' into edge

c08f878

update edge mode and tests

b5b3869

Remove NEVER()

4ae0225

cover remaining cases

018f66f

isaacbrodsky marked this pull request as ready for review December 28, 2022 17:15

isaacbrodsky added 3 commits December 28, 2022 10:29

remove commented tests

f1a28d5

fix isPentagon coverage

193727a

add coverage for new functionality in edgeLength

ff22f7e

remove irrelevant comment

4aa7e57

nrabinowitz approved these changes Jan 5, 2023

View reviewed changes

isaacbrodsky and others added 2 commits January 5, 2023 10:52

Update website/docs/library/index/h3indexing.md

d4386fd

Co-authored-by: Nick Rabinowitz <[email protected]>

Fix assertion message

c021e54

ajfriend reviewed Jan 17, 2023

View reviewed changes

dfellis reviewed Jan 17, 2023

View reviewed changes

dfellis approved these changes Jan 17, 2023

View reviewed changes

isaacbrodsky added 3 commits January 17, 2023 20:37

update isValidEdge, docs

5fd7ad4

Merge branch 'master' into edge

ce58cda

Update changelog

7aad06b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Undirected) Edge indexing mode #654

(Undirected) Edge indexing mode #654

isaacbrodsky commented Aug 24, 2022 •

edited

Loading

coveralls commented Aug 24, 2022 •

edited

Loading

nrabinowitz Sep 8, 2022

nrabinowitz Sep 8, 2022

isaacbrodsky Dec 28, 2022

nrabinowitz Sep 8, 2022

isaacbrodsky commented Dec 29, 2022

nrabinowitz left a comment

nrabinowitz Jan 5, 2023

isaacbrodsky Jan 5, 2023

nrabinowitz Jan 5, 2023

isaacbrodsky Jan 5, 2023

nrabinowitz Jan 5, 2023

ajfriend Jan 17, 2023

ajfriend Jan 17, 2023

ajfriend Jan 17, 2023

isaacbrodsky Jan 18, 2023

ajfriend commented Jan 17, 2023

dfellis Jan 17, 2023

isaacbrodsky Jan 17, 2023

dfellis Jan 17, 2023 •

edited

Loading

dfellis Jan 17, 2023

isaacbrodsky Jan 18, 2023

dfellis Jan 17, 2023

isaacbrodsky Jan 17, 2023

dfellis left a comment

CLAassistant commented Feb 16, 2023

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

(Undirected) Edge indexing mode #654

Are you sure you want to change the base?

(Undirected) Edge indexing mode #654

Conversation

isaacbrodsky commented Aug 24, 2022 • edited Loading

coveralls commented Aug 24, 2022 • 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

isaacbrodsky commented Dec 29, 2022

nrabinowitz 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

ajfriend commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfellis Jan 17, 2023 • 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

dfellis left a comment

Choose a reason for hiding this comment

CLAassistant commented Feb 16, 2023

isaacbrodsky commented Aug 24, 2022 •

edited

Loading

coveralls commented Aug 24, 2022 •

edited

Loading

dfellis Jan 17, 2023 •

edited

Loading