-
Notifications
You must be signed in to change notification settings - Fork 78
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
Accept integer input #91
Conversation
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 good so far - we'll need tests of course.
README.md
Outdated
@@ -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.<number></code> | |||
* [.h3GetFaces(h3Index)](#module_h3.h3GetFaces) ⇒ <code>[ 'Array' ].<number></code> |
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.
This suggests you have some dependency mismatch, maybe from using npm
instead of yarn
?
* 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 |
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.
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).
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 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?
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 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.
* @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; | ||
} |
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.
+1 that's basically exactly what I was thinking.
@@ -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 |
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.
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.
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.
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.
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.
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.
master:
accept-integer-input:
Nothing jumps out as a performance degradation here. Just need to add some tests to finish this PR up. |
f5ab5d7
to
15a869f
Compare
Agreed, thanks for checking. |
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.
LGTM - thanks for adding!
BTW, just checked benchmarks - this gives us a modest speedup, about 1.3x on
|
* 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
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.