Add failing test demonstrating convex hull bug

[frewsxcv]

• I agree to follow the project's code of conduct.
• I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

---

The points listed below are not contained within the convex hull

---- algorithm::convex_hull::test::convex_hull_broken stdout ----
thread 'algorithm::convex_hull::test::convex_hull_broken' panicked at 'assertion failed: `(left == right)`
  left: `[Point(Coordinate { x: -100.90146599985587, y: 29.363080999688577 }), Point(Coordinate { x: -123.09184300005427, y: 44.05538599946448 }), Point(Coordinate { x: -120.6025159999133, y: 34.68446000012909 }), Point(Coordinate { x: -123.09964799935551, y: 44.63075600008949 }), Point(Coordinate { x: -80.25807200042232, y: 25.84984799988547 }), Point(Coordinate { x: -70.29049300015168, y: 43.65201300029992 }), Point(Coordinate { x: -122.35305000035709, y: 37.93608499932344 }), Point(Coordinate { x: -99.99762699998593, y: 48.36928599964932 }), Point(Coordinate { x: -117.16856399952883, y: 32.71616400026858 }), Point(Coordinate { x: -73.4879459995081, y: 45.50278099964356 }), Point(Coordinate { x: -123.01734900015606, y: 49.22974800017022 })]`,
 right: `[]`', geo/src/algorithm/convex_hull/test.rs:854:5

[frewsxcv]

unrelated to this failing test, but because this mod test { ... } is defined here, the adjacent test.rs was previously ignored

[urschrei]

I wonder whether this is an fp precision bug. Those coordinates are awfully long.

[rmanoka]

Note that changing .contains to .intersects makes the tests pass. It may have to do with semantics of the denim predicates.

[michaelkirk]

As @rmanoka alluded, I think that the new test is invalid because polygons don't contain points on their boundary. If it did, there'd be no difference between contains and intersects. See http://lin-ear-th-inking.blogspot.be/2007/06/subtleties-of-ogc-covers-spatial.html for a longer form explanation.

A simpler example would be to consider any triangle - whose convex hull will be the triangle itself.

#[test]
fn triangle() {
    use crate::prelude::*;

    let triangle = Triangle::new((0.0, 0.0).into(), (0.0, 2.0).into(), (1.0, 1.0).into());
    let convex_hull = triangle.convex_hull();

    // the triangle (like all 2d surfaces) doesn't `contain` points on it's boundary
    assert!(!convex_hull.contains(&triangle.0));
    assert!(!convex_hull.contains(&triangle.1));
    assert!(!convex_hull.contains(&triangle.2));

    // though it does *intersect* them.
    assert!(convex_hull.intersects(&triangle.0));
    assert!(convex_hull.intersects(&triangle.1));
    assert!(convex_hull.intersects(&triangle.2));
                                                 
    // If we are talking in terms of closed 1-D geometry (like `convex_hull.exterior()`), all the points are `within` the linestring - 
    // conversely, the convex hull's exterior `contains` the points. 
    assert!(convex_hull.exterior().contains(&triangle.0));
    assert!(convex_hull.exterior().contains(&triangle.1));
    assert!(convex_hull.exterior().contains(&triangle.2));
}        

So I think we should incorporate the test fixes (thanks!) but delete or update the new failing test.

[frewsxcv]

Not suggesting we do it in this pull request, but would it be possible to add a robust convex hull algorithm? And if so, how difficult would that be?

[michaelkirk]

would it be possible to add a robust convex hull algorithm?

Good question. I'm not sure if it's currently robust or not. I know that we're using Kernel::orient2d, which is robust, but maybe there is some other math to worry about. Do you have an example input that produces unexpected results for the convex hull due to robustness issues?

Just to be clear - I don't think the issues raised by the currently failing test are related to robustness/numerical accuracy issues. Rather, I think that failure is an incorrect assumption in the new test about the (frequently surprising) definition of what contains actually means.

The test assumes that the convex hull of a set of points won't have any of those points on its boundary, but that's not how convex hulls work. Are we on the same page there @frewsxcv?

[urschrei]

I think that failure is an incorrect assumption in the new test about the (frequently surprising) definition of what contains actually means.

And how does that relate to your original issue, which looks more straightforwardly incorrect:

[michaelkirk]

To consolidate some conversation that is happening both here and in discord:

how does that relate to your original issue

@frewsxcv noticed in rgis that when using our concave hull algorithm, some points appeared outside of the hull:

Then, he created a failing unit test to encapsulate this perceived failure, that's where this PR came from.

However, the unit test was failing due to other reasons — the use of the incorrect predicate. It was using contains when it should have used intersects. When switched to using intersects, the unit test no longer fails.

Then why this observed behavior of points being rendered outside their concave hull?

I believe it's a projection issue: the data is in WGS84 (degrees) but rgis was rendering in EPSG:3857 (meters).

Operations in rgis work on whatever native coordinates the data has (i.e. WGS84), not the rendered projection (EPSG:3857). I think this is reasonable behavior, as we have to assume our input data is the most correct for whatever operations we are doing. If you want perform your operations in a different projection, explicitly project it first.

If you want to view the data in its native projection, rgis actually supports this (nice!). Using that feature you can see something more like what you might have been expecting (feel free to jump ahead to 60s for the important part):

rgis-proj.mov.mp4

Note the screen recording captures a couple UI bugs in rgis which I'll file upstream:

1. after re-projecting, I had to "zoom to layer" for the concave hull to become visible again
2. I had to switch back and forth to the window (cmd+tab) a couple times for the CRS text field to accept keyboard input.

[urschrei]

To consolidate some conversation that is happening both here and in discord:

how does that relate to your original issue

@frewsxcv noticed in rgis that when using our concave hull algorithm, some points appeared outside of the hull:

Then, he created a failing unit test to encapsulate this perceived failure, that's where this PR came from.

However, the unit test was failing due to other reasons — the use of the incorrect predicate. It was using contains when it should have used intersects. When switched to using intersects, the unit test no longer fails.

Then why this observed behavior of points being rendered outside their concave hull?

I believe it's a projection issue: the data is in WGS84 (degrees) but rgis was rendering in EPSG:3857 (meters).

Operations in rgis work on whatever native coordinates the data has (i.e. WGS84), not the rendered projection (EPSG:3857). I think this is reasonable behavior, as we have to assume our input data is the most correct for whatever operations we are doing. If you want perform your operations in a different projection, explicitly project it first.

If you want to view the data in its native projection, rgis actually supports this (nice!). Using that feature you can see something more like what you might have been expecting (feel free to jump ahead to 60s for the important part):

rgis-proj.mov.mp4

Note the screen recording captures a couple UI bugs in rgis which I'll file upstream:

1. after re-projecting, I had to "zoom to layer" for the concave hull to become visible again

2. I had to switch back and forth to the window (cmd+tab) a couple times for the CRS text field to accept keyboard input.

👑

[michaelkirk]

Superseded by #923 which included the test fixes, but not the new test.