Polygon to cells experimental fuzzer #800
Conversation
There was a problem hiding this comment.
My guess is that maxPolygonToCellsSize doesn't allocate enough cells for the new modes in some cases. I've seen similar behavior here in benchmarks - this could be an issue that affects the old polygonToCells as well.
I have a plan for a new maxPolygonToCellsSize algo that I'll try out, this may fix the fuzzer errors.
|
I think the/one issue here is polar shapes. When I run all countries, I see differences in output for Antarctica (not sure why I didn't catch this earlier): 
My new estimator seems to cover this case, though it has other issues I'm still debugging. I haven't had a chance yet to check what the old/new output looks like, though I'd guess that the new output is closer to correct due to better handling of the pole itself. |
|
Old polyfill: The new one may be correct, I'd have to import the original polygon to check |
Has this polygon been added as a test case to ensure we aren't dropping a lot of cells there? |
I'd rather add some simplified version, as the Antarctica polygon is too large to be easily added to a test file. I can add a new PR with more tests for this. |
|
Still seeing errors here with the new estimator. Is there an easy-ish way to get the input for the failure cases? |
|
I added a test case that demonstrates this. (If you run under valgrind it will show an issue.) It seems we do not actually test a polygon with a single vertex being passed in. If you do so with res=0, flags=2 (CONTAINMENT_OVERLAPPING), the estimate will be 0 but the function will try to write the buffer. |
src/h3lib/lib/polyfill.c
Outdated
| @@ -418,6 +418,12 @@ void iterStepPolygonCompact(IterCellsPolygonCompact *iter) { | |||
| iter->_started = true; | |||
| } | |||
|
|
|||
| if (iter->_polygon->geoloop.numVerts == 0) { | |||
There was a problem hiding this comment.
Should this be changed to 0 or 1 returns nothing? @nrabinowitz
There was a problem hiding this comment.
I think the case for 1 and 2 vertexes is equivalent - in both cases, we have input that cannot be a 2D polygon, and we either have to discard it or treat it like a shape. While it's obviously a pathological case, I think it would be reasonable to assume that If you have a one-vertex "polygon" and you use CONTAINMENT_OVERLAPPING that you'd get the cell that contains the point. There's also a potential use case for line segments in the 2-vertex case. I'm also concerned that the estimate and the implementation don't line up.
Should I take a shot at a PR to fix this, which we can land before this one?
There was a problem hiding this comment.
I agree that seems like the expected behavior, if you have an idea for addressing please take a look.
I can help apply these tests to another branch to help validate.
This includes the fixes from #800, plus one more fix for estimating a 1-vertex polygon. Includes the following tests: 0-vertex polygon, all modes 1-vertex polygon, all modes 2-vertex polygon, all modes polygon with 0-vertex hole, all modes polygon with 1-vertex hole, all modes polygon with 2-vertex hole, all modes
|
Here is the hexdump of a new crash case for |
|
Nick Rabinowitz seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
This adds new fuzzers for the
polygonToCellsExperimentalfunction, based on the existing functions. Since #796 (this PR is based on that branch) adds containment modes, this updates the existing fuzzers to exercise those options too.When I run the
fuzzerPolygonToCellsExperimental, I see some runtime errors about misaligned addresses, which doesn't cause an issue on Apple M1 architecture but might indicate trouble on other architectures. When I runfuzzerPolygonToCellsExperimentalNoHolesI get a crash aboutheap-buffer-overflowaroundpolygonToCellsExperimental polyfill.c:646(looks like the output exceeds the allocated output buffer in some case?)