Fix compare Ord issue #28
Conversation
Try fix suggested in stack overflow https://stackoverflow.com/questions/47489449/why-can-comparing-two-seemingly-equal-pointers-with-return-false
Allow for permutation, and rotation of ring.
Add extra test provided by @bluenote10
| "{} / {:?} / {:?} has result deviation", | ||
| filename, op, result_tag, | ||
| assert!( | ||
| mp_is_equal(result, &expected_result.result), |
There was a problem hiding this comment.
Instead of allowing for permutations, could we simply adjust the test cases so that they contain the right result permutation?
In general the Martinez algorithm should have a well-defined segment order, and in some cases it was definitely helpful to be able to match for the exact segment order to narrowly define (and study) the algorithmic behavior.
Also the reporting of a test error can get much less clear if it is order independent, because in theory we could now change the order of segments in the test cases completely and then reporting the diff will become meaningless. For instance, consider a case where the points in the expected file are [p1, p2, p3, p4] and the algorithm produces [p2, p3, p4, p1]. As long as everything matches, everything is fine. But as soon as the algorithm has a real bug, e.g. it outputs [p2, p3, p4, p1, pAdditional] the diff would show something like this:
- p1
+ p2
- p2
+ p3
- p3
+ p4
- p4
+ p1
+ pAdditional
because the diffing does not understand that we allow for permutations. In general the diff will simply mark all points as wrong, and it is unclear what really is the culprit. With exact matching we make sure that the test diffing-based test reporting works well -- in this case diffing would only report the + pAdditional line indicating one extraneous point as desired.
There was a problem hiding this comment.
Tehnically, the algo is no longer deterministic as it uses heap allocation addresses in the ordering. I think we should move to using a thread_local counter that is initialized at the beginning of algo run, and then used to give unique ids to the sweep event as they're created.
There was a problem hiding this comment.
I think we should move to using a thread_local counter that is initialized at the beginning of algo run, and then used to give unique ids to the sweep event as they're created.
Yes good idea, keeping determinism is probably desired in many use cases. I vaguely remember that I actually implemented this before. In the prepare phase of the algorithm we currently assign ring ids. I think these ring ids are not relevant anymore and we could instead assign increasing segment IDs. The idea was that instead of comparing by memory addresses, the comparison can use the stable+deterministic segment IDs.
The test contins a polygon with the same line segment repeated twice. This trips the algorithm due the `compare_segments` using addresses while the heap comparator not.
|
The changes to comparator trips the issue12 test unpredictably. The problem seems to be that issue12 shape contains a polygon with the same line segment repeated twice. This is typically considered invalid (eg. geos, JTS). In our setting, this propogates to some unpredictable decisions by the algo because the heap ordering doesn't use pointer addresses, while the compare_segments ordering does. Currently I've disabled issue12 as this is considered invalid by most libraries whereas the panic example of #17 is considered valid by shapely/geos (and presumably JTS). |
closes #17