H3 4.0.0 Part 1: Renaming otherwise unchanged functions by dfellis · Pull Request #403 · uber/h3

dfellis · 2020-10-08T22:55:49Z

Follows the renaming described here: https://github.com/uber/h3/blob/master/dev-docs/RFCs/v4.0.0/names_for_concepts_types_functions.md

coveralls · 2020-10-08T22:58:30Z

Coverage increased (+0.1%) to 99.438% when pulling 1f6a7e4 on dfellis:master into a4fda8e on uber:master.

dfellis · 2020-10-09T16:21:56Z

I believe this is ready for review!

dfellis · 2020-10-09T16:25:01Z

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

nrabinowitz

Oof! A few comments, but overall this looks great - thanks for taking this on!

CMakeLists.txt

nrabinowitz · 2020-10-14T22:29:18Z

examples/compact.c


    H3Index* compacted = calloc(inputSize, sizeof(H3Index));
-    int err = compact(input, compacted, inputSize);
+    int err = compactCells(input, compacted, inputSize);


I had forgotten all about the example files - do these not need H3_EXPORT wrapping?

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 ;) )

src/apps/applib/lib/utility.c

nrabinowitz · 2020-11-03T23:26:08Z

src/apps/filters/pointToCell.c

 * H3 indexes
 *
- *  See `geoToH3 --help` for usage.
+ *  See `pointToCell --help` for usage.


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.

Should definitely get that decided before we land this.

I would really like to know if anyone actually uses these utility programs, though.

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.

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

src/h3lib/lib/h3Index.c

src/h3lib/lib/vertex.c

src/h3lib/include/h3api.h.in

ajfriend · 2020-11-12T06:33:57Z

src/h3lib/include/h3api.h.in

+/** @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);
 /** @} */


These should just be edgeLengthRads, edgeLengthKm, and edgeLengthM, since we can drop the exact because we no longer have a naming conflict with getEdgeLengthAvg.

@dfellis bump :)

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.

isaacbrodsky

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.

isaacbrodsky · 2020-11-15T17:25:33Z

CMakeLists.txt

+    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


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.

@isaacbrodsky Could you make a follow-up issue? Or is this covered by #415?

CMakeLists.txt

src/h3lib/include/h3api.h.in

src/h3lib/lib/faceijk.c

isaacbrodsky · 2020-11-19T20:25:29Z

src/h3lib/lib/h3Index.c

 * @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; }


Seems like this should accept a resolution rather than an index.

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. :/

The proposed function name is probably fine to keep. My comment here should not block this PR, more of an off the cuff impression.

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

#443

sahrk · 2020-11-20T03:37:13Z

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

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.

sahrk · 2020-11-20T04:35:29Z

Within the function names Point is now synonymous with lat/lon. It feels a little redundant to me to then use GeoPoint for the lat/lon data type. Maybe it should just be Point?

…this PR rebased on latest

ajfriend · 2021-04-03T23:16:44Z

Within the function names Point is now synonymous with lat/lon. It feels a little redundant to me to then use GeoPoint for the lat/lon data type. Maybe it should just be Point?

~~Good point.~~ Fair. Let's consider in follow-up? #415

ajfriend

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! 🎉

dfellis force-pushed the master branch 2 times, most recently from 1b5f90c to fe192a1 Compare October 8, 2020 23:20

ajfriend self-requested a review October 8, 2020 23:46

dfellis linked an issue Oct 9, 2020 that may be closed by this pull request

Change all existing function names per naming RFC #339

Closed

dfellis changed the title ~~WIP H3 4.0.0 Part 1: Renaming otherwise unchanged functions~~ H3 4.0.0 Part 1: Renaming otherwise unchanged functions Oct 9, 2020

dfellis requested review from isaacbrodsky and nrabinowitz October 9, 2020 16:23

nrabinowitz reviewed Nov 4, 2020

View reviewed changes

nrabinowitz mentioned this pull request Nov 4, 2020

Add tests for hexRange/gridDiskUnsafe #407

Open

ajfriend reviewed Nov 12, 2020

View reviewed changes

src/h3lib/include/h3api.h.in Outdated Show resolved Hide resolved

ajfriend reviewed Nov 12, 2020

View reviewed changes

isaacbrodsky reviewed Nov 19, 2020

View reviewed changes

isaacbrodsky mentioned this pull request Dec 4, 2020

Implement a single h3 CLI entry point #415

Open

dfellis added 12 commits March 10, 2021 09:42

Rename the is* functions

6dcbf30

areNeighborCells

9cf759d

cellToParent

6fc012f

cellToCenterChild

bfe1ba6

cellToChildren and maxCellToChildrenSize

a6e55e9

getNumCells

b385f11

getRes0Cells and res0CellCount

ab7094a

getPentagons and pentagonCount

ab8fd03

getBaseCellNumber

eedb812

getResolution

9de32bb

getIcosahedronFaces

fa75214

pointToCell

b59b2ff

dfellis added 19 commits March 10, 2021 10:01

getDirectedEdgeOrigin

cbd4c63

getDirectedEdgeDestination

2bed242

getHexagonAreaAvgKm2 and getHexagonAreaAvgM2

4769e72

getHexagonEdgeLengthAvgKm and getHexagonEdgeLengthAvgM

e5606e7

cellToBoundary

554c1d7

maxGridDiskSize

d0e017d

Remove 'unidirectional' from source code

7257492

CellBoundary

01582e2

GeoPoint and LinkedGeoPoint

c509d42

GeoLoop

51b3ff9

geoloop

486cc6e

Rename some benchmarks

741d705

Rename a bunch of tests

ea05df3

Rename several filter apps

e14e7ab

Rename a couple misc apps

1d9a4f0

geoPoint.h

a021943

directedEdge.h

6763b88

Address many comments

62eebdc

Fix mistakes by the rebase algorithm and other changes needed to get …

04910c2

…this PR rebased on latest

dfellis force-pushed the master branch from 797ba89 to 04910c2 Compare March 10, 2021 18:33

pentagon -> isPent

1f6a7e4

isaacbrodsky approved these changes Mar 25, 2021

View reviewed changes

This was referenced Apr 3, 2021

Remove exact from exactEdgeLength* functions #442

Closed

Naming RFC: isResClassIII #443

Closed

Point instead of GeoPoint? #444

Closed

ajfriend approved these changes Apr 3, 2021

View reviewed changes

dfellis merged commit b744bed into uber:master Apr 7, 2021

ajfriend mentioned this pull request Apr 21, 2021

drop Cells from function names when implicit #451

Closed

ajfriend mentioned this pull request May 30, 2021

GeoPoint -> LatLng #466

Merged

Conversation

dfellis commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfellis commented Oct 9, 2020

Uh oh!

dfellis commented Oct 9, 2020

Uh oh!

nrabinowitz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaacbrodsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaacbrodsky Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajfriend Apr 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sahrk commented Nov 20, 2020

Uh oh!

sahrk commented Nov 20, 2020

Uh oh!

ajfriend commented Apr 3, 2021

Uh oh!

ajfriend left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

dfellis commented Oct 8, 2020 •

edited

Loading

coveralls commented Oct 8, 2020 •

edited

Loading

isaacbrodsky Nov 19, 2020 •

edited

Loading

ajfriend Apr 3, 2021 •

edited

Loading