Rewrite Extremes algorithm to iteratively find the extreme coordinates using CoordsIter #592
Conversation
593: Migrate CoordsIter::T to be an associated type. r=michaelkirk a=frewsxcv - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- # Previously With the previous design, this would compile: ```rust impl<'a> CoordsIter<'a, f32> for Polygon<f32> { ... } impl<'a> CoordsIter<'a, f64> for Polygon<f32> { ... } ``` Even though this is not what our implementations actually look like, to the compiler it's possible for there to be multiple implementations of `CoordsIter` for `Polygon<f32>`. This was problematic when [I started rewriting](#592) our `ExtremeIndices` trait to be generic over any type that implements `CoordsIter`: ```rust impl<'a, T, G> ExtremeIndices for G where T: CoordinateType, G: CoordsIter<'a, T>, { fn extreme_indices(&self) -> Result<Extremes, ()> { ... } } ``` This is the compilation error: ``` the type parameter `T` is not constrained by the impl trait, self type, or predicates ``` The issue is that if someone writes this with this new `ExtremeIndices` implementation: ```rust let p: Polygon<f32> = polygon![...]; p.extreme_indices() ``` The compiler doesn't know which implementation to choose: `impl<'a> CoordsIter<'a, f32> for Polygon<f32>` or `impl<'a> CoordsIter<'a, f64> for Polygon<f32>`. So we need a way to tell the compiler we only have _one_ implementation of `CoordsIter` per type. The root issue being that there is a free generic parameter in the `CoordsIter` definition. # Now To fix this, we will move the `T` type parameter to an associated type. Writing this would be a compiler error: ```rust impl<'a> CoordsIter<'a> for Polygon<f32> { type Scalar = f32; ... } impl<'a> CoordsIter<'a> for Polygon<f32> { type Scalar = f64; ... } ``` ``` conflicting implementations of trait `algorithm::coords_iter::CoordsIter<'_>` for type `geo_types::Polygon<f32>` ``` This unblocks #592 Co-authored-by: Corey Farwell <coreyf@rwell.org>
594: Add `CoordsIter::exterior_coords_iter` r=michaelkirk a=frewsxcv - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- This will get incorporated into #592 Fixes #542 ~Based on and blocked by #593 Co-authored-by: Corey Farwell <coreyf@rwell.org>
| /// | ||
| /// assert_eq!(extremes.y_max.index, 2); | ||
| /// assert_eq!(extremes.y_max.coord.x, 1.); | ||
| /// assert_eq!(extremes.y_max.coord.y, 2.); |
There was a problem hiding this comment.
It just occurred to me, but this is effectively the same thing as a bounding box, except that we also return the indices, right?
There was a problem hiding this comment.
@michaelkirk Extremes returns the most extreme coordinates of the geometry. In the case of a diamond, it will return each of the four coordinates. BoundingRect returns a minimum bounding Rect encompassing the geometry. The coordinates of the Rect are not necessarily vertices. In the case of a diamond, it will not return any of the four coordinates. The minimum bounding rectangle can be deduced from the extremes, but the reverse is not true. Does that answer your question?
There was a problem hiding this comment.
In the case of a diamond, it will return each of the four coordinates
Ah, it returns the entire coord for each extreme, not just the extreme component of the coord. Thanks for explaining!
We could similarly use exterior_coords for bounding_rect. It seems like it's already doing everything optimally, so it'd just be a little code cleanup.
I think the only case where it would be less optimal is for Rects, for which we already know the min/max. You could apply a similar optimization for Rect in extremes to save yourself a couple comparisons if you're feeling like shaving off some nanograms, but seems like small potets.
frewsxcv
changed the title
|
This is ready for review! I'll squash the commits in a little bit |
|
I'm just gonna squash with GitHub when we're ready to merge. bors try |
tryBuild succeeded: |
| find_extreme_indices(polymax_naive_indices, self) | ||
| } | ||
| } | ||
| fn extremes(&'a self) -> Option<Outcome<T>> { |
There was a problem hiding this comment.
I like the new API!
How arduous would it be to maintain a deprecated version of the old API for compatibility?
I know we're pre-v1 so technically we can "do whatever we want", but whenever we bump semver (for pre v1 like us, that's the X in 0.X.Y), people get left behind for a while, which means it's more likely they're using old code until their crate maintainers update, or are stuck with multiple incompatible versions.
That's why when it's easy, I'd prefer to do things in a way that uses deprecation so downstream users don't get stranded or need multiple versions of geo for compatibility reasons. Then we can batch all the breaking changes together, by deleting the deprecated methods, only once in a while.
I also think in-line deprecation notices are more helpful upgrade documentation than release notes for the people who update before the deprecated methods are removed.
(If it's not easy, then I'm OK with merging as is and bumping the minor version)
There was a problem hiding this comment.
Before I started implementing this, I glanced at these GitHub queries: 1 2. Assuming these are doing the right thing, there's (essentially) no open source project utilizing the old traits. There could be users, but we at least have some indication there's minimal breakage in the GeoRust ecosystem. And I also took a look at the current changelog which already has at least one breaking change, so no matter what we're gonna need to bump to 0.17.0 with the next release (unless we were to branch off master). So I'd say it's not difficult to do the deprecation dance, and I do understand the benefits, but it still takes time for myself and other maintainers. So all in all I'm neutral about doing it in this case, but I have capacity to do it if you still prefer.
|
Eh, since we’re already breaking in the next release, I’m fine with skipping it.
… On Jan 10, 2021, at 14:20, Corey Farwell ***@***.***> wrote:
@frewsxcv commented on this pull request.
In geo/src/algorithm/extremes.rs:
> {
- fn extreme_indices(&self) -> Result<Extremes, ()> {
- find_extreme_indices(polymax_naive_indices, self)
- }
-}
+ fn extremes(&'a self) -> Option<Outcome<T>> {
Before I started implementing this, I took a glance at these GitHub queries: 1 2. Assuming these are doing the right thing, there's (essentially) no open source project utilizing the old traits. There could be users, but we at least have some indication there's minimal breakage in the GeoRust ecosystem. And I also took a look at the current changelog which already has at least one breaking change, so no matter what we're gonna need to bump to 0.17.0 with the next release (unless we were to branch off master). So I'd say it's not difficult to do the deprecation dance, and I do understand the benefits, but it still takes time for myself and other maintainers. So all in all I'm neutral about doing it in this case, but I have capacity to do it if you still prefer.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Also, cool trick about using the GH search that way. I should utilize it more. |
CHANGES.mdif knowledge of this change could be valuable to users.geo::algorithm::extremes::Extremes(trait)geo::algorithm::extremes::Outcome(struct)geo::algorithm::extremes::Extreme(struct)geo::algorithm::extremes::ExtremeIndices(trait)geo::algorithm::extremes::ExtremePoints(trait)geo::Extremes(struct)The
Extremesalgorithm is now based onCoordsIterinstead ofConvexHull. The trait now runs 6x faster than the previous implementation:This speeds up any algorithms that incorporate
Extremes, likePolygontoPolygondistance: