Accept integer input #91

isaacbrodsky · 2020-09-11T01:50:07Z

This PR adds the ability to give h3-js a little endian pair of 32 bit integers. This can be useful for performance, where 32-bit integers are the output of other libs or data structures.

nrabinowitz

Looks good so far - we'll need tests of course.

nrabinowitz · 2020-09-11T16:06:38Z

README.md

@@ -91,41 +91,42 @@ const coordinates = h3.h3SetToMultiPolygon(hexagons, true);
    * [.h3IsPentagon(h3Index)](#module_h3.h3IsPentagon) ⇒ <code>boolean</code>
    * [.h3IsResClassIII(h3Index)](#module_h3.h3IsResClassIII) ⇒ <code>boolean</code>
    * [.h3GetBaseCell(h3Index)](#module_h3.h3GetBaseCell) ⇒ <code>number</code>
-    * [.h3GetFaces(h3Index)](#module_h3.h3GetFaces) ⇒ <code>Array.&lt;number&gt;</code>
+    * [.h3GetFaces(h3Index)](#module_h3.h3GetFaces) ⇒ <code>[ &#x27;Array&#x27; ].&lt;number&gt;</code>


This suggests you have some dependency mismatch, maybe from using npm instead of yarn?

nrabinowitz · 2020-09-11T16:09:05Z

lib/h3core.js

+ * 64-bit hexidecimal string representation of an H3 index,
+ * or two 32-bit integers in little endian order in an array.
+ * @static
+ * @typedef {string | number[]} H3IndexInput


Note that these get translated to Typescript defs, so we'll need to be sure they work as expected for TS (run check-tsd).

I think this would be better as [number, number] (a two-element tuple).

I would also prefer [number, number] and actually added a check for the length, too. For some reason that type is not accepted by the TypeScript compiler, perhaps it needs to be expressed in a different way for the JSDoc style definitions?

I suspect this is because JSDoc doesn't support it, not because TS doesn't. See jsdoc/jsdoc#1703

It looks like there's an ugly workaround using number[] & { 0: number, 1: number, length: 2 }, but I don't see that this would be great for legibility, so I don't think it's worth pursuing here.

nrabinowitz · 2020-09-11T16:10:53Z

lib/h3core.js

 * @return {number[]}         A two-element array with 32 lower bits and 32 upper bits
 */
 function h3IndexToSplitLong(h3Index) {
+    if (Array.isArray(h3Index) && Number.isInteger(h3Index[0]) && Number.isInteger(h3Index[1])) {
+        return h3Index;
+    }


+1 that's basically exactly what I was thinking.

nrabinowitz · 2020-09-11T16:38:50Z

lib/h3core.js

@@ -782,7 +792,7 @@ export function h3SetToMultiPolygon(h3Indexes, formatAsGeoJson) {
 * Compact a set of hexagons of the same resolution into a set of hexagons across
 * multiple levels that represents the same area.
 * @static
- * @param  {H3Index[]} h3Set H3 indexes to compact
+ * @param  {H3IndexInput[]} h3Set H3 indexes to compact


Assuming that integer mode is primarily for perf, we might consider supporting a simple array of integers here (or even a UInt32Array), in lower, upper, lower, upper, ... order. This is just extending the polymorphism of the function, so we could add it later and it would still be backwards-compatible with the array-of-arrays input.

Why not go "whole hog" with BigInt and BigUInt64Array?

There is a single example for Emscripten to access TypedArray data, so it should be relatively straightforward to efficiently pump that into the transpiled H3 and without the awkward syntax in Javascript (just put an n at the end of the integer and you're good).

The only downside is that apparently doesn't work in Safari yet? I wonder what their ETA is on fixing that, but Babel transpilation can be a temporary workaround.

Sounds good - I think we'd continue with the polymorphic approach, so it shouldn't actually matter if some platforms don't support, since the type check would only succeed if the caller passed it in - so the burden is on the caller to determine platform compatibility issues, assuming we guard against the possibility that BigInt and BigUInt64Array may not be defined.

Again, not necessary for this PR, it would be easy to extend the same polymorphic approach to these input types.

isaacbrodsky · 2020-10-09T19:55:55Z

master:

% yarn benchmark-node
yarn run v1.22.4
$ yarn rollup-benchmark-node && node dist/benchmark.node.js
$ rollup benchmark/node.js --file dist/benchmark.node.js --format=cjs --external=benchmark

benchmark/node.js → dist/benchmark.node.js...
created dist/benchmark.node.js in 600ms
h3IsValid x 3,457,025 ops/sec ±1.63% (82 runs sampled)
geoToH3 x 422,859 ops/sec ±1.51% (80 runs sampled)
h3ToGeo x 966,788 ops/sec ±0.76% (86 runs sampled)
h3ToGeoBoundary x 320,147 ops/sec ±0.49% (86 runs sampled)
h3GetFaces x 845,049 ops/sec ±0.18% (91 runs sampled)
kRing x 160,292 ops/sec ±1.01% (81 runs sampled)
polyfill_9 x 4,418 ops/sec ±1.52% (82 runs sampled)
polyfill_11 x 320 ops/sec ±0.94% (82 runs sampled)
polyfill_10ring x 67.75 ops/sec ±1.55% (67 runs sampled)
h3SetToMultiPolygon x 647 ops/sec ±1.14% (81 runs sampled)
compact x 2,159 ops/sec ±0.37% (83 runs sampled)
uncompact x 564 ops/sec ±0.40% (88 runs sampled)
h3IndexesAreNeighbors x 857,516 ops/sec ±0.54% (86 runs sampled)
getH3UnidirectionalEdge x 412,572 ops/sec ±0.87% (87 runs sampled)
getOriginH3IndexFromUnidirectionalEdge x 869,547 ops/sec ±0.56% (88 runs sampled)
getDestinationH3IndexFromUnidirectionalEdge x 789,058 ops/sec ±0.57% (89 runs sampled)
h3UnidirectionalEdgeIsValid x 2,825,928 ops/sec ±1.32% (89 runs sampled)
✨  Done in 103.76s.

accept-integer-input:

% yarn benchmark-node
yarn run v1.22.4
$ yarn rollup-benchmark-node && node dist/benchmark.node.js
$ rollup benchmark/node.js --file dist/benchmark.node.js --format=cjs --external=benchmark

benchmark/node.js → dist/benchmark.node.js...
created dist/benchmark.node.js in 615ms
h3IsValid x 3,390,848 ops/sec ±2.53% (79 runs sampled)
geoToH3 x 402,410 ops/sec ±3.23% (80 runs sampled)
h3ToGeo x 975,073 ops/sec ±2.89% (73 runs sampled)
h3ToGeoBoundary x 346,218 ops/sec ±3.19% (84 runs sampled)
h3GetFaces x 893,508 ops/sec ±1.73% (89 runs sampled)
kRing x 180,359 ops/sec ±2.85% (85 runs sampled)
polyfill_9 x 3,929 ops/sec ±4.76% (82 runs sampled)
polyfill_11 x 339 ops/sec ±3.57% (83 runs sampled)
polyfill_10ring x 63.32 ops/sec ±2.01% (65 runs sampled)
h3SetToMultiPolygon x 627 ops/sec ±3.95% (73 runs sampled)
compact x 2,088 ops/sec ±3.45% (78 runs sampled)
uncompact x 578 ops/sec ±4.03% (75 runs sampled)
h3IndexesAreNeighbors x 901,195 ops/sec ±6.22% (83 runs sampled)
getH3UnidirectionalEdge x 388,798 ops/sec ±3.11% (77 runs sampled)
getOriginH3IndexFromUnidirectionalEdge x 1,026,625 ops/sec ±2.54% (83 runs sampled)
getDestinationH3IndexFromUnidirectionalEdge x 885,489 ops/sec ±3.21% (88 runs sampled)
h3UnidirectionalEdgeIsValid x 3,041,750 ops/sec ±1.56% (81 runs sampled)
✨  Done in 103.08s.

Nothing jumps out as a performance degradation here.

Just need to add some tests to finish this PR up.

coveralls · 2020-10-09T19:59:43Z

Pull Request Test Coverage Report for Build 210

8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 202:	0.0%
Covered Lines:	395
Relevant Lines:	395

💛 - Coveralls

nrabinowitz · 2020-10-09T20:03:26Z

Nothing jumps out as a performance degradation here.

Agreed, thanks for checking.

nrabinowitz

LGTM - thanks for adding!

nrabinowitz · 2020-10-15T20:19:54Z

BTW, just checked benchmarks - this gives us a modest speedup, about 1.3x on h3ToGeo:

h3ToGeo x 1,217,239 ops/sec ±0.58% (92 runs sampled)
h3ToGeo - integers x 1,686,256 ops/sec ±0.78% (88 runs sampled)
h3ToGeoBoundary x 376,919 ops/sec ±0.49% (85 runs sampled)
h3ToGeoBoundary - integers x 404,191 ops/sec ±0.44% (91 runs sampled)

* Integer array input, as in uber/h3-js#91 * Change per review * Support Uint32Array * h3IsValid type should not throw * Add tests for H3Index parsing outside of h3IsValid * Change assertion per review

isaacbrodsky requested a review from nrabinowitz September 11, 2020 01:50

nrabinowitz approved these changes Sep 11, 2020

View reviewed changes

nrabinowitz mentioned this pull request Oct 8, 2020

Bump core library to 3.7.1, add support for cellArea, exactEdgeLength, pointDist #93

Merged

Isaac Brodsky added 3 commits October 9, 2020 12:48

Accept integer form input

967a63d

rebuild docs

9746b14

apply rebase to README

15a869f

isaacbrodsky force-pushed the accept-integer-input branch from f5ab5d7 to 15a869f Compare October 9, 2020 19:56

Isaac Brodsky added 2 commits October 15, 2020 10:02

Add tests

2408e59

Fix formatting

213aaca

isaacbrodsky marked this pull request as ready for review October 15, 2020 17:11

nrabinowitz approved these changes Oct 15, 2020

View reviewed changes

dfellis approved these changes Oct 15, 2020

View reviewed changes

nrabinowitz merged commit 5025e29 into uber:master Oct 15, 2020

nrabinowitz mentioned this pull request Oct 15, 2020

Prepare for release 3.7.0 #98

Merged

nrabinowitz mentioned this pull request Oct 31, 2020

Allow integer input for H3 indexes #90

Closed

isaacbrodsky pushed a commit to isaacbrodsky/h3-node that referenced this pull request Oct 5, 2021

Integer array input, as in uber/h3-js#91

7bd4427

isaacbrodsky mentioned this pull request Oct 5, 2021

Integer array API dfellis/h3-node#40

Merged

wiese mentioned this pull request Aug 18, 2022

Documentation of the [lat, lng] tuple #152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept integer input #91

Accept integer input #91

isaacbrodsky commented Sep 11, 2020

nrabinowitz left a comment

nrabinowitz Sep 11, 2020

nrabinowitz Sep 11, 2020

isaacbrodsky Oct 15, 2020

nrabinowitz Oct 15, 2020

nrabinowitz Sep 11, 2020

nrabinowitz Sep 11, 2020

dfellis Sep 11, 2020

nrabinowitz Sep 11, 2020

isaacbrodsky commented Oct 9, 2020 •

edited

Loading

coveralls commented Oct 9, 2020 •

edited

Loading

nrabinowitz commented Oct 9, 2020

nrabinowitz left a comment

nrabinowitz commented Oct 15, 2020

Accept integer input #91

Accept integer input #91

Conversation

isaacbrodsky commented Sep 11, 2020

nrabinowitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacbrodsky commented Oct 9, 2020 • edited Loading

coveralls commented Oct 9, 2020 • edited Loading

Pull Request Test Coverage Report for Build 210

💛 - Coveralls

nrabinowitz commented Oct 9, 2020

nrabinowitz left a comment

Choose a reason for hiding this comment

nrabinowitz commented Oct 15, 2020

isaacbrodsky commented Oct 9, 2020 •

edited

Loading

coveralls commented Oct 9, 2020 •

edited

Loading