Add h3ToIj #102

isaacbrodsky · 2018-07-26T00:53:42Z

h3ToIj wraps the internal function h3ToIjk and produces a two-dimensional IJ coordinate system. It has the same limitations of h3ToIjk regarding cells too far away from the origin or across pentagonal distortion.

Do we want to make any changes to support evolving the CoordIJ system in the future?

Builds on #83 and relates to #99.

h3ToIj wraps the internal function h3ToIjk and produces a two-dimensional IJ coordinate system. It has the same limitations of h3ToIjk regarding cells too far away from the origin or across pentagonal distortion.

coveralls · 2018-07-26T00:57:07Z

Coverage remained the same at 100.0% when pulling 8cd83bb on isaacbrodsky:h3-to-ij into dfcb55e on uber:master.

nrabinowitz · 2018-07-30T16:51:12Z

Looks good, just a few comments on documentation. I'm happy making this public if the behavior is well-defined. @dfellis or @sahrk might want to weigh in.

nrabinowitz · 2018-07-30T16:41:08Z

src/apps/testapps/testH3ToIj.c

@@ -111,6 +120,48 @@ void h3Distance_kRing_assertions(H3Index h3) {

 BEGIN_TESTS(h3ToIjk);


Nit: h3ToIj now? Not actually sure this is shown anywhere.

nrabinowitz · 2018-07-30T16:44:02Z

src/apps/testapps/testH3ToIj.c

@@ -111,6 +120,48 @@ void h3Distance_kRing_assertions(H3Index h3) {

 BEGIN_TESTS(h3ToIjk);

+H3Index bc1 = H3_INIT;
+setH3Index(&bc1, 0, 15, 0);


Nit: Can you use clearer var names or add comments explaining what these are?

nrabinowitz · 2018-07-30T16:47:05Z

src/h3lib/lib/h3Index.c

+ * @param origin An anchoring index for the ij coordinate system.
+ * @param index Index to find the coordinates of
+ * @param out ij coordinates of the index will be placed here on success
+ * @return 0 on success, or another value on failure.


Can you explain in the doc comment why you might get failure values?

dfellis

All comments are nits. Would prefer they're fixed but not blocking.

dfellis · 2018-07-30T19:35:09Z

src/apps/testapps/testH3ToIj.c

@@ -38,8 +39,11 @@ void h3Distance_identity_assertions(H3Index h3) {

    t_assert(H3_EXPORT(h3Distance)(h3, h3) == 0, "distance to self is 0");

+    // Test that coordinates are as expected
+    CoordIJ ij;
+    t_assert(H3_EXPORT(h3ToIj)(h3, h3, &ij) == 0, "failed to get ij");


The comment on this test doesn't make sense?

Clarified that comment.

dfellis · 2018-07-30T19:36:39Z

src/apps/testapps/testH3ToIj.c

-    CoordIJK origin = {0};
-    t_assert(h3ToIjk(h3, h3, &origin) == 0, "got ijk for origin");
+    CoordIJ origin = {0};
+    t_assert(H3_EXPORT(h3ToIj)(h3, h3, &origin) == 0, "got ijk for origin");


Nit: Should say got ij for origin

dfellis · 2018-07-30T19:37:02Z

src/apps/testapps/testH3ToIj.c

-        t_assert(h3ToIjk(h3, offset, &ijk) == 0, "got ijk for destination");
+        CoordIJ ij = {0};
+        t_assert(H3_EXPORT(h3ToIj)(h3, offset, &ij) == 0,
+                 "got ijk for destination");


Same nit here on ijk -> ij

dfellis · 2018-07-30T19:40:14Z

src/h3lib/include/h3api.h

+ * Each axis is spaced 120 degrees apart.
+ */
+typedef struct {
+    int i;  ///< i component


Nit: Do we really need these inline comments?

@dfellis they parallel the inline comments on components in CoordIJK, so consistency would argue for keeping them here (or changing them there). Putting them in CoordIJK was a coin-flip in favor of completeness, but I agree they're overkill.

sahrk · 2018-08-01T02:52:29Z

@isaacbrodsky asked awhile ago "Do we want to make any changes to support evolving the CoordIJ system in the future?"

You might want to specifically mention in the documentation that the result of h3ToIj for a specific pair of indexes is not guaranteed to be the same in future versions, unless you're really sure you want that to be the case.

Otherwise LGTM.

isaacbrodsky · 2018-08-20T23:44:43Z

Sorry I haven't had time to return to this PR.

Re: @sahrk

You might want to specifically mention in the documentation that the result of h3ToIj for a specific pair of indexes is not guaranteed to be the same in future versions, unless you're really sure you want that to be the case.

While I like the flexibility that gives me as a library developer, I'm concerned it might be confusing for library users. This also came up in #83 (comment), but most of the discussion there was about whether to expose h3ToIjk or h3ToIj. I'm interested in what others think of adding that caveat to the API.

dfellis · 2018-08-21T00:10:45Z

Sorry I haven't had time to return to this PR.

I've been on vacation until today so I didn't notice. ;)

Re: @sahrk

You might want to specifically mention in the documentation that the result of h3ToIj for a specific pair of indexes is not guaranteed to be the same in future versions, unless you're really sure you want that to be the case.

While I like the flexibility that gives me as a library developer, I'm concerned it might be confusing for library users. This also came up in #83 (comment), but most of the discussion there was about whether to expose h3ToIjk or h3ToIj. I'm interested in what others think of adding that caveat to the API.

I'm with @sahrk about noting that these values could change between versions of the code. As long as they're internally consistent and operations performed on them will work the same each time (eg h3ToIj(h3Index1, h3Index2,&ijVar); ijVar.i += 2; H3Index h3Index3 = ijToH3(&ijVar, h3Index1) will produce the same h3Index3) then the actual IJ values don't matter.

dfellis

Only just realized that the reverse function, ijToH3 is missing.

sahrk · 2018-08-21T02:38:49Z

ijToH3 gets to the crux of my discomfort, an ij isn’t a global location in and of itself; the user has to hang on to metadata (the origin) to get back to H3. It almost makes me want to create a struct to wrapper each ij with its origin, but that sounds inconvenient/inefficient. It might be better if users were encouraged up front to think of their ij’s as primarily local planar coordinates, with limited extent. Then when they start moving around in ij space and wander into a singularity they will be fore-warned.

dfellis · 2018-08-21T02:47:49Z

Yes, but without something like that, this library would need to implement every kind of planar operation users desire (such as line tracing that's desired for more efficient polyfill operations). Having an "out" into a coordinate system will reduce the bloating of the API.

sahrk · 2018-08-21T03:08:37Z

Agreed @dfellis, I think the utility case is very convincing. I was just encouraging more of a warning label.

nrabinowitz · 2018-08-21T16:42:35Z

To throw in my two cents here:

If we are fairly certain that the coordinate system is unstable, then I think we should explicitly mark it experimentalH3ToIj or something. As a developer, I would expect that the output of h3ToIj(h3Index1, h3Index2, &ijVar) would be stable across minor version changes, and if we're not providing that guarantee then we should be very clear about it.
Supporting currently unreachable coordinate spaces would be a minor version bump as long as working pairs of indexes continued to work in the same way. That would be fine with a comment.

As long as they're internally consistent and operations performed on them will work the same each time (eg h3ToIj(h3Index1, h3Index2,&ijVar); ijVar.i += 2; H3Index h3Index3 = ijToH3(&ijVar, h3Index1) will produce the same h3Index3) then the actual IJ values don't matter.

Is that true, or will people use the IJ values directly in their own code to implement their own algorithms in planar space? If so, I'd want a guarantee that output is stable or a very clear delineation of the unstable interfaces. I don't think a comment is sufficient unless we're willing to stick with current (successful) output until the next major version change.

isaacbrodsky · 2018-08-22T18:55:40Z

@sahrk

It might be better if users were encouraged up front to think of their ij’s as primarily local planar coordinates, with limited extent.

I definitely want to encourage that. Would changing the name of the function help, perhaps h3ToLocalIj?

I like @nrabinowitz' suggestion of adding an "experimental" prefix, which could then become a deprecated alias when the function is promoted to not have the prefix. (experimentalH3ToLocalIj?)

sahrk · 2018-08-22T23:17:09Z

@isaacbrodsky encoding a warning in the name sounds like a great idea to me. I think that adding local is sufficient to make the point, but would support either.

I realized it might be relevant that I now have the sequence number mapping mentioned in #83 implemented and will be offering it up asap. Assuming you want to use it, it seems like that should have the same kind of caveats as the ij functions, but local would seem odd since it's not really a coordinate system. Which would argue for a prefix like @nrabinowitz's experimental that could be used for both.

The function is renamed to indicate it's experimental, and the suite of functions is refactored to its own `localij.c` module.

Passing INVALID_BASE_CELL caused a lookup beyond the table, and thus undefined behavior. The behavior should always be to treat the base cell as not a pentagon in that case.

…ection is not K_AXES_DIGIT.

isaacbrodsky · 2018-09-20T21:37:37Z

Now that we have a working build, I think this is ready for a new review.

dfellis

Several comments asking for clarification, but very close to approving this.

dfellis · 2018-09-21T23:28:45Z

CHANGELOG.md

 - Benchmarks for the kRing method for k's of size 10, 20, 30, and 40.
+### Changed
+- Internal `h3ToIjk` function renamed to `h3ToLocalIjk`. (#102)


Is this supposed to be h3ToLocalIj like elsewhere or no?

No, as below the IJK functions need to be kept for internal implementation.

dfellis · 2018-09-21T23:34:53Z

src/h3lib/lib/localij.c

+ * For reversing the rotation introduced in PENTAGON_ROTATIONS when
+ * the origin is on a pentagon.
+ */
+const int PENTAGON_ROTATIONS_REVERSE[7][7] = {


I get PENTAGON_ROTATIONS, PENTAGON_ROTATIONS_REVERSE_NONPOLAR and PENTAGON_ROTATIONS_REVERSE_POLAR, but I don't get this one. Is this one applied in both cases, but then why have the other two? Or why not simply push the compound rotation results into both of the ones below if this is applied and then one of the following two are applied?

PENTAGON_ROTATIONS_REVERSE is not used in the same cases. The comments try to differentiate that this table is used when the origin index is on a pentagon base cell, the two "reverse" tables are used when the origin index is not on a pentagon base cell.

I share David's confusion - maybe expand the comments below to say ...when the index is on a pentagon and the origin is not.

dfellis · 2018-09-21T23:36:15Z

src/h3lib/lib/localij.c

+ * @param out The index will be placed here on success
+ * @return 0 on success, or another value on failure.
+ */
+int localIjkToH3(H3Index origin, const CoordIJK* ijk, H3Index* out) {


I thought IJK was going away from this diff? Or are we going H3 -> IJK -> IJ?

dfellis · 2018-09-21T23:37:09Z

src/h3lib/lib/localij.c

+    CoordIJK ijk;
+    ijToIjk(ij, &ijk);
+
+    return localIjkToH3(origin, &ijk, out);


I see, you are doing exactly that. Alright, then.

nrabinowitz

I added a few nits, but overall this is looking really good! Prestamping, assuming the nits get addressed.

nrabinowitz · 2018-09-25T19:36:04Z

src/apps/filters/h3ToLocalIj.c

@@ -14,18 +14,20 @@
 * limitations under the License.
 */
 /** @file
- * @brief stdin/stdout filter that converts from H3 indexes to relative IJK
- * coordinates. This is experimental.


Still true, yes?

nrabinowitz · 2018-09-25T19:42:51Z

src/apps/testapps/testH3ToLocalIj.c

+        t_assert(_ijkMatches(&ijk, &UNIT_VECS[0]) == 1, "not at 0,0,0 (res 0)");
+    } else if (r == 1) {
+        t_assert(_ijkMatches(&ijk, &UNIT_VECS[H3_GET_INDEX_DIGIT(h3, 1)]) == 1,
+                 "not at expected coordinates (res 1)");


Nit: It looks like these are using negative assertion messages ("test failed for condition") where most of our tests (and some in this file) use positive messages ("test succeeds for condition") or spec messages ("test must succeed when conditition").

nrabinowitz · 2018-09-25T19:45:32Z

src/apps/testapps/testH3ToLocalIj.c

+                     "test coordinates result in valid index");
+            if (_isBaseCellPentagon(H3_EXPORT(h3GetBaseCell)(testH3))) {
+                // h3IsValid doesn't test for indexes representing part of the
+                // deleted subsequence.


Should we do this? Should we add an issue for it?

I'd say such an index is invalid, similar to if the index had digits set to 7 (invalid child). I think opening an issue to make those indexes invalid would be good.

nrabinowitz · 2018-09-25T19:49:34Z

src/apps/testapps/testH3ToLocalIj.c

+        // Don't iterate all of res 3, to save time
+        iterateAllIndexesAtResPartial(3, localIjToH3_kRing_assertions, 27);
+        // These would take too long, even at partial execution
+        // iterateAllIndexesAtResPartial(4, localIjToH3_kRing_assertions, 20);


Should we include them, or just delete the commented code in this case?

Replaced in all cases with the comment that further resolutions aren't tested for time reasons.

We could also consider adding some tests to sample indexes at higher resolutions in the future.

nrabinowitz · 2018-09-25T19:51:17Z

src/h3lib/lib/algos.c

@@ -369,7 +369,7 @@ H3Index h3NeighborRotations(H3Index origin, Direction dir, int* rotations) {
        // Account for differing orientation of the base cells (this edge
        // might not follow properties of some other edges.)
        if (oldBaseCell != newBaseCell) {
-            if (newBaseCell == 4 || newBaseCell == 117) {
+            if (_isBaseCellPolarPentagon(newBaseCell)) {


nrabinowitz · 2018-09-25T19:56:17Z

src/h3lib/lib/localij.c

+    {0, -1, 0, 0, 0, 0, 0},        // 6
+};
+/**
+ * Reverse base cell direction -> leading index digit -> roations 60 ccw.


Typo, here and below: roations

nrabinowitz · 2018-09-25T19:57:29Z

src/h3lib/lib/localij.c

+ * For reversing the rotation introduced in PENTAGON_ROTATIONS when
+ * the origin is on a pentagon.
+ */
+const int PENTAGON_ROTATIONS_REVERSE[7][7] = {


I share David's confusion - maybe expand the comments below to say ...when the index is on a pentagon and the origin is not.

nrabinowitz · 2018-09-25T19:59:54Z

src/h3lib/lib/localij.c

+ */
+int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) {
+    if (H3_GET_MODE(origin) != H3_HEXAGON_MODE ||
+        H3_GET_MODE(h3) != H3_HEXAGON_MODE) {


Not sure about this check - I feel like, similar to the discussion we had around h3IsValid, it would apply to many of our functions, and it's not clear to me why we'd use it here and not in other places.

If we have the ability to validate some user inputs, is there a reason we would not want to? I think some functions aren't able to cleanly report failures right now, which certainly limits where we can add checks (until 4.0.0 and #104)

Yes, actually. If the function could be conceivably used in a looping hotspot I could see users want to have a faster iteration time if they're confident in the data already being valid.

This feels like one of those functions. H3 indexes could already be validated and stored in DB, then they want fast conversion to IJK for some planar operations and then back again.

I should have replied directly to the performance concern rather than asking a question. I think in this specific case I am OK with removing the mode check because it doesn't appear this will cause issues, for example there are no array accesses based on the mode of the indexes.

Aside: This code was not introduced in this PR, it was introduced in #83 where we had a similar discussion but didn't come to a conclusion as @nrabinowitz suggested.

I believe there are at least some inputs we should validate, a good example being where there are array accesses based on the inputs, since that could potentially result in segfault, etc.

Normalized test assertion messages to assert a positive outcome.

Tests are updated to assume that unidirectional edges are treated as their origin by this function. There does not appear to be a case where finding the IJK of the origin itself would fail, so the line checking for that in h3Distance is excluded from coverage.

isaacbrodsky · 2018-10-01T21:58:40Z

@dfellis and others, please review again.

nrabinowitz

One nit, otherwise LGTM.

nrabinowitz · 2018-10-02T19:50:15Z

src/apps/testapps/testH3ToLocalIj.c

    } else {
-        t_assert(0, "wrong resolution");
+        t_assert(0, "resolution supported by test function (coordinates)");


Nit: This (essentially assert.fail) is the one place where negative assertion messages actually make more sense IMO.

isaachier · 2018-10-03T18:01:54Z

src/h3lib/lib/localij.c

+int H3_EXPORT(h3Distance)(H3Index origin, H3Index h3) {
+    CoordIJK originIjk, h3Ijk;
+    if (h3ToLocalIjk(origin, origin, &originIjk)) {
+        // Currently there are no tests that would cause getting the coordinates


What does this mean practically? Is there no way to create a new test case that would trigger this check to fail?

As far as I can see there is no input to h3ToLocalIjk where the origin is the same as the index that would fail. The only cases that return failure involve either resolution mismatch, being on different base cells, or deleted subsequence on the pentagon (which should be unreachable when origin=index.)

I see what you mean.

Add h3ToIj

Add h3ToIj

5abcf1e

h3ToIj wraps the internal function h3ToIjk and produces a two-dimensional IJ coordinate system. It has the same limitations of h3ToIjk regarding cells too far away from the origin or across pentagonal distortion.

Fix whitespace in CMakeLists.txt

09a3bce

nrabinowitz approved these changes Jul 30, 2018

View reviewed changes

Add additional comments about failure cases

9e52b63

dfellis approved these changes Jul 30, 2018

View reviewed changes

isaacbrodsky mentioned this pull request Aug 1, 2018

Prepare for release 3.1.0 #111

Merged

Isaac Brodsky added 2 commits August 20, 2018 16:45

Adjust comments in testH3ToIj.c per review

1204338

Merge branch 'master' into h3-to-ij

60da119

dfellis requested changes Aug 21, 2018

View reviewed changes

Isaac Brodsky added 5 commits August 24, 2018 10:54

Rename h3ToIj to experimentalH3ToLocalIj

b81299d

The function is renamed to indicate it's experimental, and the suite of functions is refactored to its own `localij.c` module.

Merge branch 'master' into h3-to-ij

d1927f8

Add h3ToLocalIj to CHANGELOG

62863a4

Cleanup refactoring to localij.h

1170719

Merge branch 'master' into h3-to-ij

ffc1b32

nrabinowitz mentioned this pull request Aug 27, 2018

polyfill not filling the complete polygon #136

Closed

Isaac Brodsky added 4 commits September 14, 2018 10:52

Add experimentalLocalIjToH3

974b3f2

Merge branch 'master' into h3-to-ij

bac0ad1

Fix capitalization of localij.h

9ff9d20

Fix undefined handling of deleted base cell

a2b720d

Passing INVALID_BASE_CELL caused a lookup beyond the table, and thus undefined behavior. The behavior should always be to treat the base cell as not a pentagon in that case.

Isaac Brodsky added 2 commits September 17, 2018 11:00

Add tests for out of range local IJ coordinates, assert unrotated dir…

d722df2

…ection is not K_AXES_DIGIT.

Add test for traversing the local IJ space

3671b4e

dfellis reviewed Sep 21, 2018

View reviewed changes

nrabinowitz approved these changes Sep 25, 2018

View reviewed changes

Isaac Brodsky added 2 commits September 28, 2018 11:31

Adjust comments and test assertion messages based on feedback.

b982cb6

Normalized test assertion messages to assert a positive outcome.

dfellis approved these changes Oct 1, 2018

View reviewed changes

nrabinowitz approved these changes Oct 2, 2018

View reviewed changes

isaachier reviewed Oct 3, 2018

View reviewed changes

isaacbrodsky merged commit 36a2ceb into uber:master Oct 3, 2018

isaacbrodsky deleted the h3-to-ij branch October 3, 2018 23:58

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

Merge pull request uber#102 from isaacbrodsky/h3-to-ij

3f52fcb

Add h3ToIj

		@@ -111,6 +120,48 @@ void h3Distance_kRing_assertions(H3Index h3) {

		BEGIN_TESTS(h3ToIjk);

Add h3ToIj #102

Add h3ToIj #102

Conversation

isaacbrodsky commented Jul 26, 2018 • edited Loading

coveralls commented Jul 26, 2018 • edited Loading

nrabinowitz commented Jul 30, 2018

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

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

sahrk commented Aug 1, 2018

isaacbrodsky commented Aug 20, 2018

dfellis commented Aug 21, 2018

dfellis left a comment

Choose a reason for hiding this comment

sahrk commented Aug 21, 2018

dfellis commented Aug 21, 2018

sahrk commented Aug 21, 2018

nrabinowitz commented Aug 21, 2018

isaacbrodsky commented Aug 22, 2018

sahrk commented Aug 22, 2018 • edited Loading

isaacbrodsky commented Sep 20, 2018

dfellis 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

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

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 Oct 1, 2018

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

isaacbrodsky commented Jul 26, 2018 •

edited

Loading

coveralls commented Jul 26, 2018 •

edited

Loading

sahrk commented Aug 22, 2018 •

edited

Loading