Implement the cellToBoundary command for the h3 binary by dfellis · Pull Request #818 · uber/h3

dfellis · 2024-02-22T15:54:10Z

Just this one command at first as I remember how we had structured the arg parsing API. Tested for correctness manually, I will see if/how testing this binary was set up shortly.

coveralls · 2024-02-22T15:58:24Z

coverage: 98.827%. remained the same
when pulling 9d416a4 on h3-cellToBoundary-cmd
into a523fc6 on master.

dfellis · 2024-02-22T16:29:29Z

So... I don't see any tests for the h3 binary at all. Will like to discuss with you guys how we want to test it.

Co-authored-by: Isaac Brodsky <isaac@isaacbrodsky.com>

@sahrk

…T where applicable, and eliminate hardwired constants in favor of sizeof trick @sahrk mentioned

dfellis · 2024-03-24T01:03:22Z

So... I don't see any tests for the h3 binary at all. Will like to discuss with you guys how we want to test it.

I finally found the tests near the bottom of the CMakeTests.cmake file, and then I updated them with the new WKT formatting and added the new test. No need to bring in more complicated machinery for these tests.

isaacbrodsky

please change lat, lng ordering for compatibility with existing WKT implementations

isaacbrodsky · 2024-03-24T15:32:47Z

+    printf("%.10lf %.10lf))\n", H3_EXPORT(radsToDegs)(cb.verts[0].lng),
+           H3_EXPORT(radsToDegs)(cb.verts[0].lat));


nit/note if this function gets reused, that it depends on numVerts > 0.

Is it possible for it to be zero and not produce an H3Error value that short-circuits it, though?

As far as I know it should always have vertices if the return code is success

nrabinowitz

This looks good - most of my comments are non-blocking questions about input/output and some of the existing structure of these commands

nrabinowitz · 2024-03-25T21:21:38Z

+        return false;
+    }
+    // Using WKT formatting for the output. TODO: Add support for JSON
+    // formatting


I'm curious about plans for output and input formats here. I assume for output we'd want a -f or --format param similar to ogr2ogr. It would be easy to go overboard here, but I think at a minimum:

csv: 37.5012466151,-122.5003039349

wkt: POINT(-122.5003039349 37.5012466151)

geojson: {"type": "Point", "coordinates": [-122.5003039349 37.5012466151]}

We might also support json-array ([-122.5003039349 37.5012466151]) or json-object ({"lat":-122.5003039349, "lng": 37.5012466151}) and maybe an --ordering param specifying latlng or lnglat?

The parallel question is what inputs we support - accepting these formats as inputs is nicely symmetrical, but more painful than --lat=x --lng=y or bare arguments.

Not saying we should do any of this, but it might be nice to know where we plan to draw the line. I think a good yardstick should be how easy it is to pipe data from a tool like ogr2ogr to and from these CLI commands.

Yes, this can definitely become a rabbit hole, here. There are other things to consider, for instance: should the csv output type include a headers row for the first one with lat,lng?

But also, how much of this can we generalize across these functions, and how much has to be hand-written per sub-command? The more of the latter the less I want to support everything under the sun and just suggest users to do things like h3 cellToLatLng -c 8928342e20fffff --geojson | jq .coordinates if they want other formats.

That's part of why I was thinking of going with a JSON-based output by default, because of the possibility of mangling the format more easily than any other, but went with WKT since that's what @isaacbrodsky suggested as an expected output.

I'm not personally familiar with ogr2ogr, though, so that's part of my bias towards a JSON-based format.

nrabinowitz · 2024-03-25T21:24:54Z

-           H3_EXPORT(radsToDegs)(ll.lng));
+    H3Error err = H3_EXPORT(cellToLatLng)(cell, &ll);
+    if (err) {
+        return false;


For my info, why return true/false from these functions? Wouldn't we just want to return the error code, so callers would get a 0 exit code on success and a meaningful non-zero code on failure?

So, I think we started work on this binary before H3Error was a thing and it was doing h3IsValid checks on the inputs/outputs to determine if it should exit with an error or not.

Then the conversion of that was done this way because it was the most straightforward. Bubbling up the H3Error does make sense to me. If you don't mind, I'll keep that refactor for when I add in the next sub-command.

nrabinowitz · 2024-03-25T21:26:27Z

    H3Index c;
    H3Error e = H3_EXPORT(latLngToCell)(&ll, res, &c);

+    // TODO: Add support for JSON formatting


Does JSON formatting mean "8928342e20fffff" in this case? Also, does h3Println make piping harder than h3Print?

That or {"cell": "8928342e20fffff"}. I'm not sure which would be a better idea.

nrabinowitz · 2024-03-25T21:32:25Z

+    Arg *args[] = {&cellToBoundaryArg, &helpArg, &cellArg};
+    if (parseArgs(argc, argv, sizeof(args) / sizeof(Arg *), args, &helpArg,
+                  "Convert an H3 cell to an array of latitude/longitude "
+                  "coordinates defining its boundary")) {


Nit: Is it possible to reuse cellToBoundaryArg.helpText here instead of repeating it?

I really want to DRY this stuff up, but I think I need to rework or add some macros to do so in a way that maintains its legibility. I left it here for now because I found it easier to read the code this way, but I will do a bigger refactor in a follow-up PR that's focused just on DRYing the code so we can debate that without holding up progress on implementing a sub-command.

nrabinowitz · 2024-03-25T21:35:47Z

+        .helpText =
+            "Convert an H3 cell to an array of latitude/longitude coordinates "
+            "defining its boundary",
+    };


Nit: Is it possible to define this a top-level static const so that we can reuse it instead of repeating it?

See my prior comment. Leaving DRY-ing it to another PR focused solely on that so we can better debate the merits/long-term maintenance.

nrabinowitz · 2024-03-25T21:36:56Z

        return 0;
    }
+    if (has("cellToBoundary", 1, argv) && cellToBoundaryCmd(argc, argv)) {
+        return 0;


As above, I would have gone with return cellToBoundaryCmd(argc, argv) and have each command return H3_ERROR

I agree with you, but would like that to also be a separate PR.

Implement the cellToBoundary command for the h3 binary

65a13df

dfellis self-assigned this Feb 22, 2024

dfellis requested review from ajfriend, isaacbrodsky and nrabinowitz February 22, 2024 15:54

Fix formatting

521f667

dfellis commented Feb 22, 2024

View reviewed changes

Comment thread src/apps/filters/h3.c Outdated

dfellis commented Feb 22, 2024

View reviewed changes

Comment thread src/apps/filters/h3.c Outdated

isaacbrodsky approved these changes Feb 22, 2024

View reviewed changes

Comment thread src/apps/filters/h3.c Outdated

Comment thread src/apps/filters/h3.c Outdated

Comment thread src/apps/filters/h3.c Outdated

Comment thread src/apps/filters/h3.c Outdated

dfellis and others added 4 commits February 22, 2024 10:34

Fix error type

1c63794

Co-authored-by: Isaac Brodsky <isaac@isaacbrodsky.com>

Define a CONSTANT for the help's arg length

0b761dd

Fix formatting

4100248

Add the test for the cellToBoundary sub-command, convert output to WK…

4f5b7c9

…T where applicable, and eliminate hardwired constants in favor of sizeof trick @sahrk mentioned

isaacbrodsky requested changes Mar 24, 2024

View reviewed changes

Switch to the 'wrong' (lng, lat) ordering ;)

0447a2b

isaacbrodsky approved these changes Mar 24, 2024

View reviewed changes

nrabinowitz approved these changes Mar 25, 2024

View reviewed changes

Update help text

9d416a4

dfellis merged commit 5cd2831 into master Mar 25, 2024

dfellis deleted the h3-cellToBoundary-cmd branch March 25, 2024 23:29

		printf("%.10lf %.10lf))\n", H3_EXPORT(radsToDegs)(cb.verts[0].lng),
		H3_EXPORT(radsToDegs)(cb.verts[0].lat));

Conversation

dfellis commented Feb 22, 2024

Uh oh!

coveralls commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfellis commented Feb 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dfellis commented Mar 24, 2024

Uh oh!

isaacbrodsky left a comment

Choose a reason for hiding this comment

Uh oh!

isaacbrodsky Mar 24, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Feb 22, 2024 •

edited

Loading

isaacbrodsky Mar 24, 2024 •

edited

Loading