Skip to content
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

H3-Placekey round trip generating invalid results #44

Open
craig-walker-orennia opened this issue Apr 24, 2023 · 6 comments
Open

H3-Placekey round trip generating invalid results #44

craig-walker-orennia opened this issue Apr 24, 2023 · 6 comments

Comments

@craig-walker-orennia
Copy link

craig-walker-orennia commented Apr 24, 2023

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
Copy link
Contributor

felixsafegraph commented Apr 30, 2023

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
Copy link
Contributor

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

@craig-walker-orennia
Copy link
Author

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
Copy link
Contributor

felixsafegraph commented May 1, 2023

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
Copy link
Author

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
Copy link
Contributor

@isaacbrodsky - thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants