Remove H3IndexFat #13
Conversation
| @@ -47,6 +47,20 @@ TEST(h3IsValidDigits) { | |||
| "h3IsValid failed on invalid unused digits"); | |||
| } | |||
|
|
|||
| TEST(h3IsValidBaseCell) { | |||
There was a problem hiding this comment.
I think we're losing a test that an index with an invalid base cell (such as 127) is invalid.
There was a problem hiding this comment.
Oh? I'll look for it and include it in here. I satisfied myself with reaching 100% code coverage last night.
src/h3lib/include/h3api.h
Outdated
| @@ -43,6 +43,9 @@ | |||
| /* For size_t */ | |||
| #include <stdlib.h> | |||
|
|
|||
| /* For constants used throughout H3 */ | |||
| #include "constants.h" | |||
There was a problem hiding this comment.
Why is this include added?
There was a problem hiding this comment.
Many parts of the codebase depended on the constants being defined that they were getting indirectly through inclusion of H3IndexFat.h. It was easier to add it in here than in so many places across the codebase, and in some ways it makes sense -- if you can't #include "h3api.h" and have enough defined to be able to actually compile the requisite methods, then what kind of API is that?
There was a problem hiding this comment.
I think h3api.h exists for consumers of the library, not for the implementation, and consumers would not need these internal constants. I would prefer to replace #include "h3IndexFat.h" with #include "constants.h" if that's needed.
| "h3FatIsValid failed on large resolution"); | ||
| } | ||
|
|
||
| TEST(h3FatUnusedDigitInvalid) { |
There was a problem hiding this comment.
I think this is covered by h3IsValidDigits although not as comprehensively
| "h3FatIsValid failed on negative resolution"); | ||
| } | ||
|
|
||
| TEST(h3FatLargeResolutionInvalid) { |
There was a problem hiding this comment.
This one can't be tested on H3Index because there's only 4 bits for the resolution.
| } | ||
| } | ||
|
|
||
| TEST(h3FatNegativeResInvalid) { |
There was a problem hiding this comment.
This one can't be tested on H3Index because there's only 4 bits for the resolution.
edit: I added comments to each test, even if nothing needed to be done, just to track that I had looked at it.
src/apps/testapps/testH3Index.c
Outdated
| } | ||
| } | ||
|
|
||
| TEST(h3FatIsValidBaseCellInvalid) { |
There was a problem hiding this comment.
Sorry I missed that.
|
|
||
| BEGIN_TESTS(h3IndexFat); | ||
|
|
||
| TEST(h3FatIsValidWithMode) { |
There was a problem hiding this comment.
I don't think we have a corresponding test for this
| "h3FatIsValid failed on invalid index digit (1)"); | ||
| } | ||
|
|
||
| TEST(h3FatBadDigitInvalid) { |
There was a problem hiding this comment.
I don't think this is tested (would look like res=1 but the digits are all 7)
There was a problem hiding this comment.
Skipping the negative test (that can't exist, now) and porting the digit 7 test.
| } | ||
| } | ||
|
|
||
| TEST(h3FatIsValidNegativeBaseCellInvalid) { |
There was a problem hiding this comment.
This can't be expressed in H3Index
| t_assert(!h3FatIsValid(&hfLarge), "h3FatIsValid failed on too large digit"); | ||
| } | ||
|
|
||
| TEST(h3FatIsValidBaseCell) { |
There was a problem hiding this comment.
This test is ported over.
| "h3FatIsValid failed on negative base cell"); | ||
| } | ||
|
|
||
| TEST(h3FatIsValidBaseCellInvalud) { |
There was a problem hiding this comment.
This test is ported over.
| "h3FatIsValid failed on invalid base cell"); | ||
| } | ||
|
|
||
| TEST(setH3IndexFat) { |
There was a problem hiding this comment.
This is already tested on H3Index (setH3Index)
src/apps/testapps/testH3Index.c
Outdated
| H3Index h = H3_INIT; | ||
| H3_SET_MODE(h, i); | ||
| char failureMessage[BUFF_SIZE]; | ||
| sprintf(failureMessage, "h3FatIsValid failed on mode %d", i); |
| * and normalized base cell coordinates. | ||
| * @return Returns 1 if the possibility of overage exists, otherwise 0. | ||
| */ | ||
| int _h3ToFaceIjkWithInitializedFijk(H3Index h, FaceIJK* fijk) { |
There was a problem hiding this comment.
nit: s/_h3ToFaceIjkWithInitializedFijk/_h3ToFaceIJKWithInitializedFIJK and same for others
There was a problem hiding this comment.
So that's how the original versions of these functions were named in H3IndexFat, and it makes a kind of sense to camelCase that way since it would be obvious to split as "h3 to face ijk with initialized fijk" whereas the other way would split as "h3 to face i j k with initialized f i j k".
There was a problem hiding this comment.
If that’s how it was, no need to bunch unrelated changes into this. Comment rescinded!
I don’t buy the Ijk makes more sense than IJK argument, though :P
|
Wasn't sure if you guys wanted to manage the CHANGELOG yourselves or allow contributors to (since my other diffs didn't get entries). Since they're all focused on eliminating H3IndexFat, that should still be fine. :) |
|
I guess we don’t have a policy for who adds to the changelog. My preference is commiter should add an entry under unreleased then We can groom the changelog on bump as needed. But I’ll raise this point up to others at our next meeting. No need to pollute your PR :) Thanks @dfellis!! |
Remove H3IndexFat
Made the change to remove H3IndexFat completely from the H3!
This change depends on the work in #12 so that has to be landed first, then I can rebase this on latest master (it should rebase perfectly). This also eliminates
h3Api.cand appears to speed up H3 by 9% according to the benchmarks. (I haven't even gotten started on all of the perf stuff I want to do. I think the IJK rotation logic could be reduced from a loop to an O(1) algo and speed many things up.)