-
Notifications
You must be signed in to change notification settings - Fork 455
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
H3 4.0.0 Part 1: Renaming otherwise unchanged functions #403
Conversation
1b5f90c
to
fe192a1
Compare
I believe this is ready for review! |
@sahrk I can't formally assign you as a reviewer, but in case you are interested, we're finally getting started on cleaning up the naming convention for a v4 release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof! A few comments, but overall this looks great - thanks for taking this on!
@@ -35,7 +35,7 @@ int main(int argc, char* argv[]) { | |||
printf("Starting with %d indexes.\n", inputSize); | |||
|
|||
H3Index* compacted = calloc(inputSize, sizeof(H3Index)); | |||
int err = compact(input, compacted, inputSize); | |||
int err = compactCells(input, compacted, inputSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had forgotten all about the example files - do these not need H3_EXPORT
wrapping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They use H3 as a library, so H3_EXPORT
is not available, and the test compiles it with an empty prefix so this works (or the test suite would be complaining ;) )
@@ -17,7 +17,7 @@ | |||
* @brief stdin/stdout filter that converts from lat/lon coordinates to integer | |||
* H3 indexes | |||
* | |||
* See `geoToH3 --help` for usage. | |||
* See `pointToCell --help` for usage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth noting that the CLI utils is one place where the more generic names aren't helpful - if I have a util in my path called geoToH3
I know what it relates to, but pointToCell
is much more ambiguous. I wonder if we want an h3
prefix on the filter applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should definitely get that decided before we land this.
I would really like to know if anyone actually uses these utility programs, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in the steering committee. @isaacbrodsky suggests consolidating these in a single filter command like h3 pointToCell <...args>
. This would be a separate PR, obviously, so what you have here seems sufficient for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dfellis I use these :). I can't be the only one that uses pipes in the unix shell to transform data.
I concur with @nrabinowitz that the "CLI utils is one place where the more generic names aren't helpful". Within H3 itself the decision was made to make lat/lon equivalent to point
, etc. But the command line user is outside H3 trying to transform between specific location representations, and using the abstract terms obscures that.
/** @brief exact length for a specific directed edge in radians*/ | ||
double H3_EXPORT(exactEdgeLengthRads)(H3Index edge); | ||
|
||
/** @brief exact length for a specific unidirectional edge in kilometers*/ | ||
/** @brief exact length for a specific directed edge in kilometers*/ | ||
double H3_EXPORT(exactEdgeLengthKm)(H3Index edge); | ||
|
||
/** @brief exact length for a specific unidirectional edge in meters*/ | ||
/** @brief exact length for a specific directed edge in meters*/ | ||
double H3_EXPORT(exactEdgeLengthM)(H3Index edge); | ||
/** @} */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should just be edgeLengthRads
, edgeLengthKm
, and edgeLengthM
, since we can drop the exact
because we no longer have a naming conflict with getEdgeLengthAvg
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dfellis bump :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, never mind. I see that these aren't in the RFC. I'll make an issue for a follow-up PR to change these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize for the delay in reviewing this PR. Thanks for pushing this part of the 4.0 effort forward.
I was able to complete reading through it and I think it makes sense to me. I see Nick and AJ have already commented on a few things, and I agree especially with the gridDiskDistancesUnsafe comment (or the one where the arguments were more complicated than needed), etc.
src/apps/filters/cellToPoint.c | ||
src/apps/filters/h3ToLocalIj.c | ||
src/apps/filters/localIjToH3.c | ||
src/apps/filters/h3ToComponents.c | ||
src/apps/filters/geoToH3.c | ||
src/apps/filters/h3ToGeoBoundary.c | ||
src/apps/filters/kRing.c | ||
src/apps/filters/hexRange.c | ||
src/apps/filters/pointToCell.c | ||
src/apps/filters/cellToBoundary.c | ||
src/apps/filters/gridDisk.c | ||
src/apps/filters/gridDiskUnsafe.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest leaving the names of the filters as-is and they can be refactored in a subsequent PR. I don't think this needs to be removed from this PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isaacbrodsky Could you make a follow-up issue? Or is this covered by #415?
* Class III resolution (rotated versus the icosahedron and subject | ||
* to shape distortion adding extra points on icosahedron edges, making | ||
* them not true hexagons). | ||
* @param h The H3Index to check. | ||
* @return Returns 1 if the hexagon is class III, otherwise 0. | ||
*/ | ||
int H3_EXPORT(h3IsResClassIII)(H3Index h) { return H3_GET_RESOLUTION(h) % 2; } | ||
int H3_EXPORT(isResClassIII)(H3Index h) { return H3_GET_RESOLUTION(h) % 2; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this should accept a resolution rather than an index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is letting people know if the hexagon is a class III hexagon. I would prefer it to just be isClassIII
and return isResDigitClassIII
to this name, but that isn't what we agreed upon in the RFC and I think the relitigation of these decisions all the time is why 4.0 is taking so long to get out the door. :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed function name is probably fine to keep. My comment here should not block this PR, more of an off the cuff impression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to revisit in a follow-up. I could debate names all day 😛 .
I think these changes have taken so long just due to the scale and the coupling between name changes. After this PR lands, I think @dfellis has done most of the hard work. I'd expect follow-up adjustments like this to be smaller-scale and less-coupled, so we should be able to resolve them fairly quickly. 🙏
Thanks for the heads up @dfellis, and sorry for being late to the party. I am looking at it now and will make any comments this evening. Overall it looks great; I'm especially happy to have a standard naming for "ring" and "disk" that can be adopted outside of H3 going forward. |
Within the function names |
…this PR rebased on latest
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for doing all the hard work @dfellis!
I've tried to move any outstanding discussions to issues/PRs that we can follow up on, so that we can finally land this thing! 🎉
Follows the renaming described here: https://github.com/uber/h3/blob/master/dev-docs/RFCs/v4.0.0/names_for_concepts_types_functions.md