Placekey / placekey-js

https://placekey.io
Apache License 2.0
22 stars 4 forks source link

H3-Placekey round trip generating invalid results #44

Open craig-walker-orennia opened 1 year ago

craig-walker-orennia commented 1 year ago

We've discovered that converting a valid H3 Index to a Placekey and then converting it back generates an invalid H3 Index. Here's a test example:

import { h3ToPlacekey, placekeyToH3 } from '@placekey/placekey'
import { isValidCell } from "h3-js"

const h3Input = "847b59dffffffff"
const placekey = h3ToPlacekey(h3Input)
const h3Output = placekeyToH3(placekey)
console.log({
  validInput: isValidCell(h3Input),
  h3Input,
  placekey,
  h3Output,
  validOutput: isValidCell(h3Output),
})

Output:

{
  validInput: true,
  h3Input: '847b59dffffffff',
  placekey: '@ff7-swh-m49',
  h3Output: '8a7b59dffffffff',
  validOutput: false
}

Note that the output index is only different in the 2nd digit; 4 changes to an a.

We first discovered this using the Carto spatial library for Snowflake, which uses Placekey and H3 under the covers: https://docs.carto.com/data-and-analysis/analytics-toolbox-for-snowflake/sql-reference/placekey. The same bug exists there.

felixsafegraph commented 1 year ago

thank you for reporting.

it looks like your h3 input 847b59dffffffff is at resolution 4 (bigger area)? https://h3geo.org/docs/core-library/restable/

what the library currently does at h3 cell resolution 10 8a7b59dffffffff

felixsafegraph commented 1 year ago

perhaps you can tell us how you get the h3 input - that might point us to the reason

craig-walker-orennia commented 1 year ago

That particular H3 index was one of the ones we have in our system; I just started testing the Placekey API with it.

what the library currently does at h3 cell resolution 10

Does this mean that Placekey only works with indexes that are of resolution 10?

felixsafegraph commented 1 year ago

that particular behavior might be undefined - we might not have discussed what should happen if an user provide a h3 cell (I assume it should snap to the resolution 10 cell)

as of now Placekey is at res 10. its design can handle res 10-12. if you need a bigger area, have you consider https://h3geo.org/docs/api/regions/ ?

craig-walker-orennia commented 1 year ago

We don't need a bigger area for our purposes. Mostly our H3 indexes are stand-ins for actual lat/longs so scaling them down is easy.

Placekey not supporting resolutions higher than 10 is fine. But this behaviour isn't showing that. Placekey will happily generate a valid-looking Placekey value from an H3 index of resolution 4. But Placekey can't take this valid-looking value and turn it into a valid H3 index; instead it returns a garbage value. Better behaviour would be:

  1. If Placekey is intentionally not supporting H3 resolutions larger than 10, than either return null or throw an exception when receiving one.
  2. Placekey's API should never return an invalid Placekey.
  3. If there is such a thing as an invalid Placekey, then there should be an API that I can call to determine whether a Placekey is valid or not. So far I don't see one, and so there's no way for me to tell whether the Placekey I've generated is valid or not.
  4. If I do pass an invalid Placekey in to placekeyToH3, it should return null or throw an exception, not return a result.
  5. placekeyToH3 should never be returning invalid H3 indexes.
felixsafegraph commented 1 year ago

@isaacbrodsky - thoughts?