Robust ExtremePoint trait #505

rmanoka · 2020-09-04T07:35:53Z

This PR introduces IsConvex trait, and partially revamps the ExtremePoint trait.

add IsConvex trait and documentation of predicates.
add vector space ops in Coordinate. These were earlier
available in Point, but it makes more sense to be
available for Coordinate too.
many minor doc improvements: explain corner cases,
better references, fix spellings (collinear).
make ExtremePoint robust
Remove unneeded trait generic parameter T, and requirement T: Float

Things to revamp / discuss in ExtremePoint

FIgure out what the trait is about. Should we really compute convex_hull in the impl?
Replace Result<Extremes, ()> with Option

rmanoka · 2020-09-04T07:45:35Z

@urschrei @frewsxcv The Polygon type has an is_convex method that is somewhat buggy: is not robust, and handles collinear points in an unexpected manner (it allows collinear pts if oriented cw, but not otherwise). Also, the usual math. defn. of convex also requires that the polygon has no interior holes. Further, we can not directly make it robust as the kernels are part of geo crate, and not geo-types.

Should we remove that functionality from geo-types in favor of introducing it as a trait in geo? This way, we can add a more exhaustive list of (robust) predicates: is_strongly_ccw_convex is_strongly_cw_convex is_strongly_convex is_ccw_convex is_cw_convex is_convex.

It is only used in a couple of geo traits, and we can fix that. Is a breaking change though as it is a pub fn.

michaelkirk · 2020-09-07T15:26:10Z

It is only used in a couple of geo traits, and we can fix that. Is a breaking change though as it is a pub fn.

This makes sense to me. It would be kinder to deprecate it, change the internal impls to use the robust versions, and remove it after some time has passed.

michaelkirk · 2020-09-07T20:30:54Z

geo-types/src/coordinate.rs

+
+    /// Construct a point at the origin, that is, with both
+    /// `x` and `y` coordinates as zero.
+    pub fn zero() -> Self {


❤️ I've wanted this little method many times.

1. add `IsConvex` trait and documentation of predicates. 1. add vector space ops in `Coordinate`. These were earlier available in `Point`, but it makes more sense to be available for `Coordinate` too. 1. many minor doc improvements: explain corner cases, better references, fix spellings (collinear). Oh, btw, please skip ci !

rmanoka · 2020-09-07T20:40:31Z

@michaelkirk @urschrei I'm quite confused about the ExtremePoint trait. The implementation seems to simply compute the coordinates with min-x, min-y, max-x, and max-y. However, for doing this, it seems to be running the convex hull algorithm! Note that convex hull is O(n logn) while just direct search would just be O(n), so I have no idea what is happening.

For now, I'm going to leave this trait as is, and just make a few calls robust. Some clarity on the purpose of this trait would be very helpful.

…x/robust-extremes

Impl `Div`, `Mul` by scalar for both `Coordinate` and `Point`.

urschrei · 2020-09-07T23:01:49Z

Two things:

ExtremePoint was an add-on to ExtremeIndices, and the fact that it could be implemented generically is actually a big disadvantage here, as you indirectly point out: in many cases, there's no need to ensure convexity.
2. When it comes to collinearity, are you referring to sequential collinear points, or within some epsilon? I ask because the geometry model (JTS and GEOS, again) allows you to assume no repeating adjacent points in a geometry. Of course, we kicked the can down the road by writing non-validating constructors because we were worried about performance…

edit: no more late-night posting.

rmanoka · 2020-09-08T02:44:29Z

@urschrei Got it. We currently use polymax_naive but could switch to the binary search version at some point. Still, we can't compute convex_hull just for this purpose, as we might as well just do a loop through instead. The binary search version could probably be provided as an independent algo. that expects (but does not verify) its input to be a convex linestring.

+ remove `Float` requirement + doc fixes for stable rust + remove `<T>` from `Extreme{Point,Indices}`

rmanoka · 2020-09-08T03:07:23Z

On a side note: the check whether to use the "rotating calipers" fast path in euclidean_distance.rs may be wrong:

if poly2.is_convex() || !self.is_convex()

Shouldn't it check if both are convex? It seems to want one of them to not be convex.

michaelkirk

LGTM!

All my feedback was optional.

geo-types/src/coordinate.rs

geo/src/algorithm/winding_order.rs

geo-types/src/polygon.rs

michaelkirk · 2020-09-08T17:26:36Z

geo/src/algorithm/extremes.rs

-        Point::new(T::one(), T::zero()),
-        Point::new(T::zero(), T::one()),
-        Point::new(-T::one(), T::zero()),
+    let directions: Vec<Coordinate<_>> = vec![


no biggie so take it or leave it, but since this is fixed size we could make this an array instead of a vec.

I don't think fixed size works here because it is not an iterator. We can re-visit this once we have the ForEachCoord trait, and simplify the logic here.

geo/src/algorithm/is_convex.rs

michaelkirk · 2020-09-08T17:57:39Z

geo/src/algorithm/is_convex.rs

+                if allow_collinear {
+                    None
+                } else {
+                    Some((i, orientation))


A little clarity nit:

At this point, find_first_non_collinear is in fact collinear, right?

To me that reads like let not_a_dog = 🐕;

Would something like starting_orientation be clearer than find_first_non_collinear?

No biggie though, it just tripped me up as I was trying to understand.

So, the first loop is in fact finding the first non-collinear orientation; we return collinear in that loop only if !allow_collinear, and as a fast exit path. The apparent inversion is unfortunately how find / find_map works: it shorts at the first Some() return from the closure.

geo/src/algorithm/euclidean_distance.rs

+ deprecate `Polygon::is_convex` in favour of the robust algorithm in geo + fix fast path condition in euclidean distance between `Polygon`s + doc fixes

rmanoka · 2020-09-08T18:36:37Z

bors r=michaelkirk

bors · 2020-09-08T18:48:26Z

Build succeeded:

scalar mul/div for point

c96060d

rmanoka mentioned this pull request Sep 7, 2020

scalar mul/div for point #507

Closed

michaelkirk reviewed Sep 7, 2020

View reviewed changes

rmanoka force-pushed the fix/robust-extremes branch from 9922800 to 9ea0c90 Compare September 7, 2020 20:31

rmanoka added 2 commits September 8, 2020 02:11

Merge remote-tracking branch 'origin/mkirk/scalar-point-math' into fi…

e37c805

…x/robust-extremes

Add more standard ops to Coordinate, Point

2b957bd

Impl `Div`, `Mul` by scalar for both `Coordinate` and `Point`.

rmanoka mentioned this pull request Sep 7, 2020

Combine Orient and WindingOrder algorithms? #364

Open

IsConvex for robust check if linestring is convex

be61add

rmanoka marked this pull request as ready for review September 7, 2020 21:58

Make Extreme{Point,Indices} work on integers

23b9740

+ remove `Float` requirement + doc fixes for stable rust + remove `<T>` from `Extreme{Point,Indices}`

michaelkirk approved these changes Sep 8, 2020

View reviewed changes

michaelkirk mentioned this pull request Sep 8, 2020

only use "rotating calipers" distance calculation when both polygons are convex #509

Closed

rmanoka force-pushed the fix/robust-extremes branch from efde663 to 1e32bcb Compare September 8, 2020 18:20

michaelkirk reviewed Sep 8, 2020

View reviewed changes

geo/src/algorithm/euclidean_distance.rs Show resolved Hide resolved

Minor fixes

f5d1349

+ deprecate `Polygon::is_convex` in favour of the robust algorithm in geo + fix fast path condition in euclidean distance between `Polygon`s + doc fixes

rmanoka force-pushed the fix/robust-extremes branch from 1e32bcb to f5d1349 Compare September 8, 2020 18:32

bors bot merged commit ea5ce7e into georust:master Sep 8, 2020

rmanoka mentioned this pull request Sep 9, 2020

Use robust crate for orientation tests #415

Closed

6 tasks

rmanoka deleted the fix/robust-extremes branch September 17, 2020 03:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Robust ExtremePoint trait #505

Robust ExtremePoint trait #505

rmanoka commented Sep 4, 2020 •

edited

Loading

rmanoka commented Sep 4, 2020 •

edited

Loading

michaelkirk commented Sep 7, 2020

michaelkirk Sep 7, 2020

rmanoka commented Sep 7, 2020

urschrei commented Sep 7, 2020 •

edited

Loading

rmanoka commented Sep 8, 2020

rmanoka commented Sep 8, 2020 •

edited

Loading

michaelkirk left a comment

michaelkirk Sep 8, 2020

rmanoka Sep 8, 2020

michaelkirk Sep 8, 2020

rmanoka Sep 8, 2020

rmanoka commented Sep 8, 2020

bors bot commented Sep 8, 2020

Robust ExtremePoint trait #505

Robust ExtremePoint trait #505

Conversation

rmanoka commented Sep 4, 2020 • edited Loading

rmanoka commented Sep 4, 2020 • edited Loading

michaelkirk commented Sep 7, 2020

michaelkirk Sep 7, 2020

Choose a reason for hiding this comment

rmanoka commented Sep 7, 2020

urschrei commented Sep 7, 2020 • edited Loading

rmanoka commented Sep 8, 2020

rmanoka commented Sep 8, 2020 • edited Loading

michaelkirk left a comment

Choose a reason for hiding this comment

michaelkirk Sep 8, 2020

Choose a reason for hiding this comment

rmanoka Sep 8, 2020

Choose a reason for hiding this comment

michaelkirk Sep 8, 2020

Choose a reason for hiding this comment

rmanoka Sep 8, 2020

Choose a reason for hiding this comment

rmanoka commented Sep 8, 2020

bors bot commented Sep 8, 2020

rmanoka commented Sep 4, 2020 •

edited

Loading

rmanoka commented Sep 4, 2020 •

edited

Loading

urschrei commented Sep 7, 2020 •

edited

Loading

rmanoka commented Sep 8, 2020 •

edited

Loading