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

Test cellsToLinkedMultiPolygon for children of pentagon #916

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ajfriend
Copy link
Contributor

@ajfriend ajfriend commented Sep 26, 2024

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

coveralls commented Sep 27, 2024

Coverage Status

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

Comment on lines +76 to +80
// children of pentagon 0x80ebfffffffffff
H3Index kids[] = {0x81ea3ffffffffff, 0x81eabffffffffff,
0x81eafffffffffff, 0x81eb3ffffffffff,
0x81eb7ffffffffff, 0x81ebbffffffffff};
int numCells = ARRAY_SIZE(kids);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this happen for any set of res 1 children of a res 0 pentagon?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the notebook here: https://gist.github.com/ajfriend/5594157463b88eb8d4cae35705657d8d

At res 1, it only happens for 80ebfffffffffff. (but it also happens for res 2... let me get a list)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At res 2, exactly two pentagons of the 12 do not work: 8009fffffffffff and 80c3fffffffffff

@heshpdx
Copy link
Contributor

heshpdx commented Sep 30, 2024

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

isaacbrodsky commented Sep 30, 2024

I suspect this has to do with

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

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

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

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

grim7reaper commented Sep 30, 2024

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.

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

Successfully merging this pull request may close these issues.

6 participants