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 h3ToIj #102

Merged
merged 18 commits into from
Oct 3, 2018
Merged
Show file tree
Hide file tree
Changes from 3 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
8 changes: 4 additions & 4 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ set(EXAMPLE_SOURCE_FILES
examples/edge.c)
set(OTHER_SOURCE_FILES
src/apps/filters/h3ToGeo.c
src/apps/filters/h3ToIjk.c
src/apps/filters/h3ToIj.c
src/apps/filters/h3ToComponents.c
src/apps/filters/geoToH3.c
src/apps/filters/h3ToGeoBoundary.c
Expand Down Expand Up @@ -158,7 +158,7 @@ set(OTHER_SOURCE_FILES
src/apps/testapps/mkRandGeo.c
src/apps/testapps/testH3Api.c
src/apps/testapps/testH3SetToLinkedGeo.c
src/apps/testapps/testH3ToIjk.c
src/apps/testapps/testH3ToIj.c
src/apps/miscapps/h3ToGeoBoundaryHier.c
src/apps/miscapps/h3ToGeoHier.c
src/apps/miscapps/generateBaseCellNeighbors.c
Expand Down Expand Up @@ -281,7 +281,7 @@ endmacro()
add_h3_executable(geoToH3 src/apps/filters/geoToH3.c ${APP_SOURCE_FILES})
add_h3_executable(h3ToComponents src/apps/filters/h3ToComponents.c ${APP_SOURCE_FILES})
add_h3_executable(h3ToGeo src/apps/filters/h3ToGeo.c ${APP_SOURCE_FILES})
add_h3_executable(h3ToIjk src/apps/filters/h3ToIjk.c ${APP_SOURCE_FILES})
add_h3_executable(h3ToIj src/apps/filters/h3ToIj.c ${APP_SOURCE_FILES})
add_h3_executable(h3ToGeoBoundary src/apps/filters/h3ToGeoBoundary.c ${APP_SOURCE_FILES})
add_h3_executable(hexRange src/apps/filters/hexRange.c ${APP_SOURCE_FILES})
add_h3_executable(kRing src/apps/filters/kRing.c ${APP_SOURCE_FILES})
Expand Down Expand Up @@ -457,7 +457,7 @@ if(BUILD_TESTING)
add_h3_test(testBBox src/apps/testapps/testBBox.c)
add_h3_test(testVec2d src/apps/testapps/testVec2d.c)
add_h3_test(testVec3d src/apps/testapps/testVec3d.c)
add_h3_test(testH3ToIjk src/apps/testapps/testH3ToIjk.c)
add_h3_test(testH3ToIj src/apps/testapps/testH3ToIj.c)

add_h3_test_with_arg(testH3NeighborRotations src/apps/testapps/testH3NeighborRotations.c 0)
add_h3_test_with_arg(testH3NeighborRotations src/apps/testapps/testH3NeighborRotations.c 1)
Expand Down
20 changes: 10 additions & 10 deletions src/apps/filters/h3ToIjk.c → src/apps/filters/h3ToIj.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,17 @@
* limitations under the License.
*/
/** @file
* @brief stdin/stdout filter that converts from H3 indexes to relative IJK
* coordinates. This is experimental.
* @brief stdin/stdout filter that converts from H3 indexes to relative IJ
* coordinates.
*
* usage: `h3ToIjk [origin]`
* usage: `h3ToIj [origin]`
*
* The program reads H3 indexes from stdin and outputs the corresponding
* IJK coordinates to stdout, until EOF is encountered. The H3 indexes
* should be in integer form. `-1 -1 -1` is printed if the IJK coordinates
* IJ coordinates to stdout, until EOF is encountered. The H3 indexes
* should be in integer form. `NA` is printed if the IJ coordinates
* could not be obtained.
*
* `origin` indicates the origin (or anchoring) index for the IJK coordinate
* `origin` indicates the origin (or anchoring) index for the IJ coordinate
* space.
*/

Expand All @@ -37,11 +37,11 @@
#include "utility.h"

void doCell(H3Index h, H3Index origin) {
CoordIJK ijk;
if (h3ToIjk(origin, h, &ijk)) {
printf("-1 -1 -1\n");
CoordIJ ij;
if (H3_EXPORT(h3ToIj)(origin, h, &ij)) {
printf("NA\n");
} else {
printf("%d %d %d\n", ijk.i, ijk.j, ijk.k);
printf("%d %d\n", ij.i, ij.j);
}
}

Expand Down
97 changes: 76 additions & 21 deletions src/apps/testapps/testH3ToIjk.c → src/apps/testapps/testH3ToIj.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@
* limitations under the License.
*/
/** @file
* @brief tests H3 index to IJK+ grid functions.
* @brief tests H3 index to IJ or IJK+ grid functions, and H3 distance
* function.
*
* usage: `testH3ToIjk`
* usage: `testH3ToIj`
*/

#include <stdio.h>
Expand All @@ -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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment on this test doesn't make sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clarified that comment.

CoordIJK ijk;
t_assert(h3ToIjk(h3, h3, &ijk) == 0, "failed to get ijk");
ijToIjk(&ij, &ijk);
if (r == 0) {
t_assert(_ijkMatches(&ijk, &UNIT_VECS[0]) == 1, "not at 0,0,0 (res 0)");
} else if (r == 1) {
Expand All @@ -57,8 +61,10 @@ void h3Distance_identity_assertions(H3Index h3) {
}

void h3Distance_neighbors_assertions(H3Index h3) {
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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Should say got ij for origin

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

CoordIJK originIjk;
ijToIjk(&origin, &originIjk);

for (int d = 1; d < 7; d++) {
if (d == 1 && h3IsPentagon(h3)) {
Expand All @@ -68,8 +74,11 @@ void h3Distance_neighbors_assertions(H3Index h3) {
int rotations = 0;
H3Index offset = h3NeighborRotations(h3, d, &rotations);

CoordIJK ijk = {0};
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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same nit here on ijk -> ij

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

CoordIJK ijk;
ijToIjk(&ij, &ijk);
CoordIJK invertedIjk = {0};
_neighbor(&invertedIjk, d);
for (int i = 0; i < 3; i++) {
Expand All @@ -78,7 +87,7 @@ void h3Distance_neighbors_assertions(H3Index h3) {
_ijkAdd(&invertedIjk, &ijk, &ijk);
_ijkNormalize(&ijk);

t_assert(_ijkMatches(&ijk, &origin), "back to origin");
t_assert(_ijkMatches(&ijk, &originIjk), "back to origin");
}
}

Expand Down Expand Up @@ -109,7 +118,51 @@ void h3Distance_kRing_assertions(H3Index h3) {
}
}

BEGIN_TESTS(h3ToIjk);
BEGIN_TESTS(h3ToIj);

// Some indexes that represent base cells. Base cells
// are hexagons except for `pent1`.
H3Index bc1 = H3_INIT;
setH3Index(&bc1, 0, 15, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


H3Index bc2 = H3_INIT;
setH3Index(&bc2, 0, 8, 0);

H3Index bc3 = H3_INIT;
setH3Index(&bc3, 0, 31, 0);

H3Index pent1 = H3_INIT;
setH3Index(&pent1, 0, 4, 0);

TEST(ijkToIj_zero) {
CoordIJK ijk = {0};
CoordIJ ij = {0};

ijkToIj(&ijk, &ij);
t_assert(ij.i == 0, "ij.i zero");
t_assert(ij.j == 0, "ij.j zero");

ijToIjk(&ij, &ijk);
t_assert(ijk.i == 0, "ijk.i zero");
t_assert(ijk.j == 0, "ijk.j zero");
t_assert(ijk.k == 0, "ijk.k zero");
}

TEST(ijkToIj_roundtrip) {
for (Direction dir = CENTER_DIGIT; dir < NUM_DIGITS; dir++) {
CoordIJK ijk = {0};
_neighbor(&ijk, dir);

CoordIJ ij = {0};
ijkToIj(&ijk, &ij);

CoordIJK recovered = {0};
ijToIjk(&ij, &recovered);

t_assert(_ijkMatches(&ijk, &recovered),
"got same ijk coordinates back");
}
}

TEST(testIndexDistance) {
H3Index bc = 0;
Expand Down Expand Up @@ -193,18 +246,6 @@ TEST(h3Distance_kRing) {
}

TEST(h3DistanceBaseCells) {
H3Index bc1 = H3_INIT;
setH3Index(&bc1, 0, 15, 0);

H3Index bc2 = H3_INIT;
setH3Index(&bc2, 0, 8, 0);

H3Index bc3 = H3_INIT;
setH3Index(&bc3, 0, 31, 0);

H3Index pent1 = H3_INIT;
setH3Index(&pent1, 0, 4, 0);

t_assert(H3_EXPORT(h3Distance)(bc1, pent1) == 1,
"distance to neighbor is 1 (15, 4)");
t_assert(H3_EXPORT(h3Distance)(bc1, bc2) == 1,
Expand All @@ -231,4 +272,18 @@ TEST(h3DistanceFailed) {
"cannot compare at different resolutions");
}

TEST(h3ToIjFailed) {
CoordIJ ij;

t_assert(H3_EXPORT(h3ToIj)(bc1, bc1, &ij) == 0, "failed to find IJ (1)");
t_assert(ij.i == 0 && ij.j == 0, "ij correct (1)");
t_assert(H3_EXPORT(h3ToIj)(bc1, pent1, &ij) == 0, "failed to find IJ (2)");
t_assert(ij.i == 1 && ij.j == 0, "ij correct (2)");
t_assert(H3_EXPORT(h3ToIj)(bc1, bc2, &ij) == 0, "failed to find IJ (3)");
t_assert(ij.i == 0 && ij.j == -1, "ij correct (3)");
t_assert(H3_EXPORT(h3ToIj)(bc1, bc3, &ij) == 0, "failed to find IJ (4)");
t_assert(ij.i == -1 && ij.j == 0, "ij correct (4)");
t_assert(H3_EXPORT(h3ToIj)(pent1, bc3, &ij) != 0, "failed to find IJ (5)");
}

END_TESTS();
5 changes: 5 additions & 0 deletions src/h3lib/include/coordijk.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,13 @@
#define COORDIJK_H

#include "geoCoord.h"
#include "h3api.h"
#include "vec2d.h"

/** @struct CoordIJK
* @brief IJK hexagon coordinates
*
* Each axis is spaced 120 degrees apart.
*/
typedef struct {
int i; ///< i component
Expand Down Expand Up @@ -102,5 +105,7 @@ void _ijkRotate60cw(CoordIJK* ijk);
Direction _rotate60ccw(Direction digit);
Direction _rotate60cw(Direction digit);
int ijkDistance(const CoordIJK* a, const CoordIJK* b);
void ijkToIj(const CoordIJK* ijk, CoordIJ* ij);
void ijToIjk(const CoordIJ* ij, CoordIJK* ijk);

#endif
18 changes: 18 additions & 0 deletions src/h3lib/include/h3api.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,16 @@ struct LinkedGeoPolygon {
LinkedGeoPolygon *next;
};

/** @struct CoordIJ
* @brief IJ hexagon coordinates
*
* Each axis is spaced 120 degrees apart.
*/
typedef struct {
int i; ///< i component
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Do we really need these inline comments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

int j; ///< j component
} CoordIJ;

/** @defgroup geoToH3 geoToH3
* Functions for geoToH3
* @{
Expand Down Expand Up @@ -449,6 +459,14 @@ void H3_EXPORT(getH3UnidirectionalEdgeBoundary)(H3Index edge, GeoBoundary *gb);
int H3_EXPORT(h3Distance)(H3Index origin, H3Index h3);
/** @} */

/** @defgroup h3ToIj h3ToIj
* Functions for h3ToIj
* @{
*/
/** @brief Returns two dimensional coordinates for the given index */
int H3_EXPORT(h3ToIj)(H3Index origin, H3Index h3, CoordIJ *out);
/** @} */

#ifdef __cplusplus
} // extern "C"
#endif
Expand Down
27 changes: 27 additions & 0 deletions src/h3lib/lib/coordijk.c
Original file line number Diff line number Diff line change
Expand Up @@ -504,4 +504,31 @@ int ijkDistance(const CoordIJK* c1, const CoordIJK* c2) {
_ijkNormalize(&diff);
CoordIJK absDiff = {abs(diff.i), abs(diff.j), abs(diff.k)};
return MAX(absDiff.i, MAX(absDiff.j, absDiff.k));
}

/**
* Transforms coordinates from the IJK+ coordinate system to the IJ coordinate
* system.
*
* @param ijk The input IJK+ coordinates
* @param ij The output IJ coordinates
*/
void ijkToIj(const CoordIJK* ijk, CoordIJ* ij) {
ij->i = ijk->i - ijk->k;
ij->j = ijk->j - ijk->k;
}

/**
* Transforms coordinates from the IJ coordinate system to the IJK+ coordinate
* system.
*
* @param ij The input IJ coordinates
* @param ijk The output IJK+ coordinates
*/
void ijToIjk(const CoordIJ* ij, CoordIJK* ijk) {
ijk->i = ij->i;
ijk->j = ij->j;
ijk->k = 0;

_ijkNormalize(ijk);
}
32 changes: 32 additions & 0 deletions src/h3lib/lib/h3Index.c
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,9 @@ int isResClassIII(int res) { return res % 2; }
* Coordinates are only comparable if they come from the same
* origin index.
*
* Failure may occur if the index is too far away from the origin
* or if the index is on the other side of a pentagon.
*
* @param origin An anchoring index for the ijk+ coordinate system.
* @param index Index to find the coordinates of
* @param out ijk+ coordinates of the index will be placed here on success
Expand Down Expand Up @@ -960,6 +963,35 @@ int h3ToIjk(H3Index origin, H3Index h3, CoordIJK* out) {
return 0;
}

/**
* Produces ij coordinates for an index anchored by an origin.
*
* The coordinate space used by this function may have deleted
* regions or warping due to pentagonal distortion.
*
* Coordinates are only comparable if they come from the same
* origin index.
*
* Failure may occur if the index is too far away from the origin
* or if the index is on the other side of a pentagon.
*
* @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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

*/
int H3_EXPORT(h3ToIj)(H3Index origin, H3Index h3, CoordIJ* out) {
CoordIJK ijk;
int failed = h3ToIjk(origin, h3, &ijk);
if (failed) {
return failed;
}

ijkToIj(&ijk, out);

return 0;
}

/**
* Produces the grid distance between the two indexes.
*
Expand Down