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.

Sign up for GitHub

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

Jump to bottom

Consistent pentagon local IJ coords #234

Merged
merged 5 commits into from
May 28, 2019
Merged

Consistent pentagon local IJ coords #234

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4f4f4f9
Consistent pentagon local IJ coords
May 3, 2019
b6ba77b
Merge branch 'master' into local-ij-origin-dependent
May 16, 2019
d9bcfbb
Adjust comments and changelog entry
May 16, 2019
b440ffb
formatting
May 16, 2019
8399808
Adjust comment about pentagon unfolding
May 17, 2019
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
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ The public API of this library consists of the functions declared in file

## [Unreleased]
### Changed
- All dynamic internal memory allocations happen on the heap instead of the stack
- Local coordinate spaces cannot cross more than one icosahedron edge. (#234)
- All dynamic internal memory allocations happen on the heap instead of the stack. (#235)

## [3.4.3] - 2019-05-02
### Added
Expand Down
8 changes: 8 additions & 0 deletions src/apps/testapps/testH3Line.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,12 @@ SUITE(h3Line) {
iterateAllIndexesAtResPartial(3, h3Line_kRing_assertions, 6);
// Further resolutions aren't tested to save time.
}

TEST(h3Line_acrossMultipleFaces) {
H3Index start = 0x85285aa7fffffff;
H3Index end = 0x851d9b1bfffffff;

int lineSz = H3_EXPORT(h3LineSize)(start, end);
t_assert(lineSz < 0, "Line not computable across multiple icosa faces");
}
}
70 changes: 70 additions & 0 deletions src/apps/testapps/testH3ToLocalIj.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,4 +341,74 @@ SUITE(h3ToLocalIj) {
t_assert(H3_EXPORT(experimentalH3ToLocalIj)(pent1, bc3, &ij) != 0,
"found IJ (5)");
}

/**
* Test that coming from the same direction outside the pentagon is handled
* the same as coming from the same direction inside the pentagon.
*/
TEST(onOffPentagonSame) {
for (int bc = 0; bc < NUM_BASE_CELLS; bc++) {
for (int res = 1; res <= MAX_H3_RES; res++) {
// K_AXES_DIGIT is the first internal direction, and it's also
// invalid for pentagons, so skip to next.
Direction startDir = K_AXES_DIGIT;
if (_isBaseCellPentagon(bc)) {
startDir++;
}

for (Direction dir = startDir; dir < NUM_DIGITS; dir++) {
H3Index internalOrigin;
setH3Index(&internalOrigin, res, bc, dir);

H3Index externalOrigin;
setH3Index(&externalOrigin, res,
_getBaseCellNeighbor(bc, dir), CENTER_DIGIT);

for (Direction testDir = startDir; testDir < NUM_DIGITS;
testDir++) {
H3Index testIndex;
setH3Index(&testIndex, res, bc, testDir);

CoordIJ internalIj;
int internalIjFailed =
H3_EXPORT(experimentalH3ToLocalIj)(
internalOrigin, testIndex, &internalIj);
CoordIJ externalIj;
int externalIjFailed =
H3_EXPORT(experimentalH3ToLocalIj)(
externalOrigin, testIndex, &externalIj);

t_assert(
(bool)internalIjFailed == (bool)externalIjFailed,
"internal/external failed matches when getting IJ");

if (internalIjFailed) {
continue;
}

H3Index internalIndex;
int internalIjFailed2 =
H3_EXPORT(experimentalLocalIjToH3)(
internalOrigin, &internalIj, &internalIndex);
H3Index externalIndex;
int externalIjFailed2 =
H3_EXPORT(experimentalLocalIjToH3)(
externalOrigin, &externalIj, &externalIndex);

t_assert(
(bool)internalIjFailed2 == (bool)externalIjFailed2,
"internal/external failed matches when getting "
"index");

if (internalIjFailed2) {
continue;
}

t_assert(internalIndex == externalIndex,
"internal/external index matches");
}
}
}
}
}
}
62 changes: 28 additions & 34 deletions src/h3lib/lib/localij.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,24 +87,28 @@ const int PENTAGON_ROTATIONS_REVERSE_POLAR[7][7] = {
{0, 1, 1, 0, 1, 1, 1}, // 6
};

// Simply prohibit many pentagon distortion cases rather than handling them.
const bool FAILED_DIRECTIONS_II[7][7] = {
{false, false, false, false, false, false, false}, // 0
{false, false, false, false, false, false, false}, // 1
{false, false, false, false, true, false, false}, // 2
{false, false, false, false, false, false, true}, // 3
{false, false, false, true, false, false, false}, // 4
{false, false, true, false, false, false, false}, // 5
{false, false, false, false, false, true, false}, // 6
};
const bool FAILED_DIRECTIONS_III[7][7] = {
/**
* Prohibited directions when unfolding a pentagon.
*
* Indexes by two directions, both relative to the pentagon base cell. The first
* is the direction of the origin index and the second is the direction of the
* index to unfold. Direction refers to the direction from base cell to base
* cell if the indexes are on different base cells, or the leading digit if
* within the pentagon base cell.
*
* This previously included a Class II/Class III check but these were removed
* due to failure cases. It's possible this could be restricted to a narrower
* set of a failure cases. Currently, the logic is any unfolding across more
* than one icosahedron face is not permitted.
*/
const bool FAILED_DIRECTIONS[7][7] = {
{false, false, false, false, false, false, false}, // 0
{false, false, false, false, false, false, false}, // 1
{false, false, false, false, false, true, false}, // 2
{false, false, false, false, true, false, false}, // 3
{false, false, true, false, false, false, false}, // 4
{false, false, false, false, false, false, true}, // 5
{false, false, false, true, false, false, false}, // 6
{false, false, false, false, true, true, false}, // 2
{false, false, false, false, true, false, true}, // 3
{false, false, true, true, false, false, false}, // 4
{false, false, true, false, false, false, true}, // 5
{false, false, false, true, false, true, false}, // 6
};

/**
Expand Down Expand Up @@ -183,15 +187,9 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) {
if (originOnPent) {
int originLeadingDigit = _h3LeadingNonZeroDigit(origin);

// TODO: This previously included the Class III-based checks
// as in the index-on-pentagon case below, but these were
// removed due to some failure cases. It is possible that we
// could restrict this error to a narrower set of cases.
// https://github.com/uber/h3/issues/163
if (FAILED_DIRECTIONS_III[originLeadingDigit][dir] ||
FAILED_DIRECTIONS_II[originLeadingDigit][dir]) {
// TODO this part of the pentagon might not be unfolded
// correctly.
if (FAILED_DIRECTIONS[originLeadingDigit][dir]) {
// TODO In this case this part of the pentagon might not be
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: In this case this part... -> In case this part...

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I think it's correct as is - "in case" means you don't know whether something might occur, but you want to anticipate it; "in this case" means you are clear that in the current context something will occur, and you are handling it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But we don't know if it's actually unfolding incorrectly or not, so in case that would happen we're doing something. The current phrasing is very confusing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I requested this change, because every time I see this comment I think that return 3 is thought to be possibly incorrect, instead of being a correct guard against possible issues. Open to other options. Maybe

TODO: We may be unfolding the pentagon incorrectly in this case; return an error
code until this is guaranteed to be correct.

?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That one is definitely clear.

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'll update with the revised wording @nrabinowitz suggested

// unfolded correctly.
return 3;
}

Expand All @@ -200,12 +198,9 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) {
} else if (indexOnPent) {
int indexLeadingDigit = _h3LeadingNonZeroDigit(h3);

if ((isResClassIII(res) &&
FAILED_DIRECTIONS_III[indexLeadingDigit][revDir]) ||
(!isResClassIII(res) &&
FAILED_DIRECTIONS_II[indexLeadingDigit][revDir])) {
// TODO this part of the pentagon might not be unfolded
// correctly.
if (FAILED_DIRECTIONS[indexLeadingDigit][revDir]) {
// TODO In this case this part of the pentagon might not be
// unfolded correctly.
return 4;
}

Expand Down Expand Up @@ -248,9 +243,8 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) {
int originLeadingDigit = _h3LeadingNonZeroDigit(origin);
int indexLeadingDigit = _h3LeadingNonZeroDigit(h3);

if (FAILED_DIRECTIONS_III[originLeadingDigit][indexLeadingDigit] ||
FAILED_DIRECTIONS_II[originLeadingDigit][indexLeadingDigit]) {
// TODO this part of the pentagon might not be unfolded
if (FAILED_DIRECTIONS[originLeadingDigit][indexLeadingDigit]) {
// TODO In this case this part of the pentagon might not be unfolded
// correctly.
return 5;
}
Expand Down