uber / h3

Hexagonal hierarchical geospatial indexing system
https://h3geo.org
Apache License 2.0
4.95k stars 470 forks source link

Test `cellsToLinkedMultiPolygon` for children of pentagon #916

Open ajfriend opened 2 months ago

ajfriend commented 2 months ago

Adds a test that catches a current bug when computing the cellsToLinkedMultiPolygon of a set of children of a pentagon cell.

Parent pentagon: 80ebfffffffffff Children: 81ea3ffffffffff, 81eabffffffffff, 81eafffffffffff, 81eb3ffffffffff, 81eb7ffffffffff, 81ebbffffffffff

image
coveralls commented 2 months ago

Coverage Status

coverage: 98.825%. remained the same when pulling 61fc849c0aa1ea80f5e2660d0d6e75ac5218e043 on aj/loop_bug into 9cc20fd36d4d9f25832aa1fb638b880571aa4262 on master.

heshpdx commented 2 months ago

In case it helps, I ran make test with this branch on two aarch64 Ampere machines running linux, and everything passed including testCellsToLinkedMultiPolygon_test95. I tried on Altra and AmpereOne.

isaacbrodsky commented 2 months ago

I suspect this has to do with https://github.com/uber/h3/blob/b355c9d825428b8f08636ffa7250422f52c2f6e0/src/h3lib/lib/vertexGraph.c#L76

For example, changing it from

    return (uint32_t)fmod(fabs((vertex->lat + vertex->lng) * pow(10, 15 - res)),
                          numBuckets);

to

    //                                                               ||
    //                                                               vv
    return (uint32_t)fmod(fabs((vertex->lat + vertex->lng) * pow(10, 16 - res)),
                          numBuckets);

seems to resolve this particular error case. I think we had considered using vertex indexes for this algorithm before, but hadn't done so. If I recall right the performance wasn't much different (wasn't better anyways). I think @nrabinowitz did those tests so I'll let him chime in. Do we still have that on a branch we could resurrect?

nrabinowitz commented 2 months ago

I think we had considered using vertex indexes for this algorithm before, but hadn't done so. If I recall right the performance wasn't much different (wasn't better anyways). I think @nrabinowitz did those tests so I'll let him chime in. Do we still have that on a branch we could resurrect?

I'm... not sure, I'll need to check. It is possible my WIP branch (now several years old) went away with an old work laptop :/.

nrabinowitz commented 2 months ago

I think we had considered using vertex indexes for this algorithm before, but hadn't done so. If I recall right the performance wasn't much different (wasn't better anyways). I think @nrabinowitz did those tests so I'll let him chime in. Do we still have that on a branch we could resurrect?

I'm... not sure, I'll need to check. It is possible my WIP branch (now several years old) went away with an old work laptop :/.

Does not seem to be in my remote GH branches. I think it's unlikely I can access the code any more, sorry about that.

grim7reaper commented 2 months ago

Not sure if it could help to implement it in C, but the h3o implementation is based on Vertex indexes.

IIRC, it wasn't too complicated to adapt the algorithm, the only tricky part being the "distortions" vertices that needs to be added and cannot be represented by the vertex indexes, which only encode topological vertices. I got sth working at the end of the day, but not very satisfied of the approach so there may be room for improvements.