relative_eq() #567

martinfrances107 · 2020-12-11T16:18:38Z

Added num_coords() method to MultiPoint. To Point, Line, LineString and MultiPoint test helpers and associated tests were added to assert that two structs were relatively equal. That is identical within the limits of numerical rounding accuracy.

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.

geo-types/src/point.rs

michaelkirk

Hi @martinfrances107 - this is looking pretty good!

With these traits implemented, we can now use something like:

approx::assert_relative_eq!(line_string_1, line_string_2);

Right? It'd be nice to see that macro usage in a test, since that's how we usually access these traits.

Is there a reason not to implement this on all of geo_types::Geometry?

Other than that, I have left some (I think) small requests for changes.

michaelkirk · 2020-12-15T15:06:38Z

geo-types/src/line.rs

+        epsilon: Self::Epsilon,
+        max_relative: Self::Epsilon,
+    ) -> bool {
+        self.start_point()


Rather than line.start_point()/line.end_point() which construct a Points, can we instead compare the underlying Coordinates directly?

I'd prefer something like this:

self.0.relative_eq(other.0, epsilon, max_relative) && self.1.relative_eq(other.1, epsilon, max_relative)

I have fixed this in the commit, to this PR, labelled 'points.rs Simplify RelativeEq for Point'

I want to respond to this question

With these traits implemented, we can now use something like:
approx::assert_relative_eq!(line_string_1, line_string_2);
Right?

I have added a new commit here "Added new feature relative_eq"
It does not work as I want yet... so there is more to come.

But I am at the point where I should discuss some architectural re-plumbing in public.

I actually want this to work in a module I am developing. This project rightly wants to keep approx as an internal dev-dependency. So If I want to see in in my third party module then I need to optionally expose approx when the other build environment ( Cargo.toml) enables the geo module's feature "relative_eq" This might explain some of my changes.

My current issue is that I can ONLY demonstrate approx::assert_relative_eq working in a limited number of build environments. It works in 'test' and 'doctest' ONLY and ONLY inside geo-types. When I try and demo this in geo sub-crate ( ie geo/src/algorithm/rotate.rs) or my third party module.. then it fails to compile..

michaelkirk · 2020-12-15T15:06:58Z

geo-types/src/line.rs

+
+    #[inline]
+    fn abs_diff_eq(&self, other: &Line<T>, epsilon: Self::Epsilon) -> bool {
+        self.start_point()


ditto about point vs. coords

Thanks you fixed in : -
lines.rs Simplify "AbsDiffEq for Line" to operate of coords not points.

michaelkirk · 2020-12-15T15:10:34Z

geo-types/src/line_string.rs

+where
+    T: CoordinateType + Float + AbsDiffEq,
+{
+    // type Epsilon = T;


Does this comment mean something or just leftover and can can be removed?

Thanks .. I will fix this

michaelkirk · 2020-12-15T15:11:12Z

geo-types/src/line_string.rs

+    }
+
+    #[inline]
+    fn abs_diff_eq(&self, other: &LineString<T>, epsilon: Self::Epsilon) -> bool {


Can we do the num_coords check optimization here too like you did in relative_eq?

Thanks this was fixed in the commit

line_string.rs - abs_diff_eq() Now rejects line_strings with unequal lengths. (with tests)

geo-types/src/line_string.rs

geo-types/src/multi_point.rs

geo-types/src/point.rs

michaelkirk · 2020-12-15T15:23:25Z

geo-types/src/point.rs

@@ -409,6 +414,79 @@ where
    }
 }

+#[cfg(test)]
+impl<T> RelativeEq for Point<T>


Please put the implementation on Coordinate and change Point's implementation to delegate to that - like:

impl<T> RelativeEq for Point<T> { fn relative_eq(self, other, epsilon) -> bool { self.0.relative_eq(other.0, epsilon) } ... }

please put the implementation on

I think when I submit a fix... there is going to be a radical change here. So I want to signal it ahead of time

in the main issue .. I talked about euclidean distance versus 'Manhattan' distance.

Coordinate{x: T, y: T} has such a similar structure to Complex{ re: T, im: T}

I think I made a over-complication when I look in approx-0.4..0 at the implementation of Complex I see this ..

The implementation forgets the euclidean distance and just ensures that the both re and im meet the constraint individually. Because I copy/modifed the default implementation .. the same infinity checks for example will be done in the same order .. etc

#[cfg(feature = "num-complex")] impl<T: RelativeEq> RelativeEq for Complex<T> where T::Epsilon: Clone, { #[inline] fn default_max_relative() -> T::Epsilon { T::default_max_relative() } #[inline] fn relative_eq( &self, other: &Complex<T>, epsilon: T::Epsilon, max_relative: T::Epsilon, ) -> bool { T::relative_eq(&self.re, &other.re, epsilon.clone(), max_relative.clone()) && T::relative_eq(&self.im, &other.im, epsilon.clone(), max_relative.clone()) } }

Honestly - I think using manhattan distance is fine — it might even be preferred because it's so simple.

Great -- so this has been fixed. The implementation was not moved into coords .. it was simply dissolved away.

I'd actually still prefer if this were moved into Coordinate as described, even if the implementation is much simpler now.

Reason being that Coordinate is kind of the currency of lots of geo algorithms, so it'd be great if we could start to leverage this functionality in those tests too.

Thank you .. yep I had only sorted this out for RelativeEq... While I was fixing this - I noticed that I was systematically over constraining this stuff with 'Float' ... fixed.

this commit.

Removed type constraint on RelativeEq and AbsDiffEq that the type must be Float. Also AbsDiffEq for Point now uses AbsDiffEq in Coords.

michaelkirk · 2020-12-15T15:36:28Z

geo-types/src/multi_point.rs

+    /// let multi_point: MultiPoint<f32> = coords.into_iter().collect();
+    /// assert_eq!(3, multi_point.num_coords());
+    /// ```
+    pub fn num_coords(&self) -> usize {


This is a helpful method.

Since @frewsxcv has a WIP implementation for the same functionality here: #563, I think it'd be better to avoid the public API churn - can you make this method private or pub(crate) for now, and add a comment linking to the as-of-yet unmerged PR to explain why it's not public.

BTW #563 has since merged.

Could you please rebase or merge against master and use the newly added CoordsIter#coords_count method instead of this one?

There is a question of placement, I would like to discuss..

CoordsItrer#coords_count is defined in the geo module.

And there is a security conflict. The implementation of the trait must be in the module defining the trait or the module containing the struct upon which the trait operates.

The conflict is that I can't

A) use coords_count() in geo_types.

or

B) I can't define RelatveEq for Point in geo because it it viewed as a unsafe third party module.

I will post and update when I have found it ... but I can't do what you ask.

So I have rebased and resolved the deprection notices. here

"After rebase, drop the use of the deprecated num_coords() method."

The solution is that coords_count is a simple one-liner - so I opted to break the DRY principle and inline the function is a few places .. no biggie

I think this will lead to frustration for our users down the road to have methods with identical functionality but different names. I don't think people will be able to remember that all geo::Geometries can use coords_count but that only some geo::Geometries can use num_coords.

As you rightly point out, we unfortunately can't use the geo::CoordsIter trait in the geo-types crate, so instead I think it's best to delete this num_coords method and instead use the more verbose self.0.len() in the geo-types crate.

michaelkirk · 2020-12-15T15:47:16Z

geo-types/src/point.rs

+        epsilon: Self::Epsilon,
+        max_relative: Self::Epsilon,
+    ) -> bool {
+        // Handle same infinities.


I was initially surprised that we weren't simply delegating to the approx::relative_eq, but I think I understand why.

It might be helpful for future readers if there were a comment that explained this - something like:

// this implementation is based on the [approx crates `relative_eq`](some link) // implementation, but rather than comparing the difference // between two scalars, we compare the euclidean distance between two // coordinates

(feel free to improve)

martinfrances107 · 2020-12-16T12:57:07Z

I am slowly fixing up the code in response to the review. I will post again when I think I have gotten to all the problems.

michaelkirk · 2020-12-16T22:41:00Z

Looks like you're making steady progress @martinfrances107! Please tag me when you're ready for another round of review.

…nd MultiPoint test helpers and associated tests were added to assert that two structs were relatively equal. That is identical within the limits of numerical rounding accuracy.

…ints.

…lengths. (with tests)

…t be Float. Also AbsDiffEq for Point now uses AbsDiffEq in Coords.

martinfrances107 · 2020-12-24T11:58:52Z

@michaelkirk , Hi

I think I have fixed all the outstanding issues - and this is PR is ready for a review again

I have modified geo/src/algorithm/rotate.rs to use the new assertions and avoid hardcoding numerical rounding errors in the expected test results.

I have checked cargo doc ... and looked at the HTML output .. example of how to used the new methods are documented.

I wanted to be able to demonstrate these test / dependencies features being used my third party module but I think I want to leave that as a followup.

Thanks for the detailed review it really did greatly simplify the code and caught some holes in the test strategy

Merry Christmas!

michaelkirk

This is looking very close!

I had some additional feedback and am hoping to solicit some input from other georust folks in one comment in particular...

michaelkirk · 2020-12-29T17:55:15Z

geo-types/src/line.rs

+    /// let a = Line::new(Coordinate { x: 0., y: 0. }, Coordinate { x: 1., y: 1. });
+    /// let b = Line::new(Coordinate { x: 0., y: 0. }, Coordinate { x: 1.001, y: 1. });
+    ///
+    /// approx::assert_relative_eq!(a, b, epsilon=0.1)


assert_abs_diff_eq

michaelkirk · 2020-12-29T17:55:42Z

geo-types/src/line.rs

+        T::default_epsilon()
+    }
+
+    /// Equality assertion within a relative limit.


Comment seems like its for relative_eq not abs_diff_eq

michaelkirk · 2020-12-29T17:56:35Z

geo-types/src/line_string.rs

+    /// let mut coords_b = vec![(0., 0.), (5., 0.), (7.001, 9.)];
+    /// let b: LineString<f32> = coords_b.into_iter().collect();
+    ///
+    /// approx::assert_relative_eq!(a, b, epsilon=0.1)


ditto about assert_abs_diff_eq and the comment

michaelkirk · 2020-12-29T18:02:16Z

geo-types/src/multi_point.rs

+    /// let multi_point: MultiPoint<f32> = coords.into_iter().collect();
+    /// assert_eq!(3, multi_point.num_coords());
+    /// ```
+    pub fn num_coords(&self) -> usize {


I think this will lead to frustration for our users down the road to have methods with identical functionality but different names. I don't think people will be able to remember that all geo::Geometries can use coords_count but that only some geo::Geometries can use num_coords.

As you rightly point out, we unfortunately can't use the geo::CoordsIter trait in the geo-types crate, so instead I think it's best to delete this num_coords method and instead use the more verbose self.0.len() in the geo-types crate.

michaelkirk · 2020-12-29T18:08:44Z

geo-types/src/multi_point.rs

+    /// let a = MultiPoint(vec![point![x: 0., y: 0.], point![x: 10., y: 10.]]);
+    /// let b = MultiPoint(vec![point![x: 0., y: 0.], point![x: 10.001, y: 10.]]);
+    ///
+    /// approx::assert_relative_eq!(a, b, epsilon=0.1)


ditto about assert_abs_diff_eq and the comment

michaelkirk · 2020-12-29T18:10:12Z

geo/Cargo.toml

@@ -22,19 +22,20 @@ geographiclib-rs = { version = "0.2" }

 proj = { version = "0.20.3", optional = true }

-geo-types = { version = "0.6.2", path = "../geo-types", features = ["rstar"] }
+geo-types = { version = "0.6.2", optional = true, path = "../geo-types", features = ["rstar"] }


I'm not 100% I understand the changes in this file. Surely geo cannot be built without geo-types, so making it an optional dependency seems fraught.

I'm guessing this is related to enabling the relative_eq by default only as a devDependency... Is that right? I see why that might be tricky.

A maybe radical alternative which avoids the problem is to get rid of the relative_eq feature you've introduced and simply have the new functionality you've written enabled by default.

I appreciate the intent, because we try to keep geo-types svelte, but since geo-types already unconditionally relies on the approx crate, including your new code for the approx impls seems like an acceptable addition to me.

Anyone else care to weigh in?
/cc @georust/geo-publishers

Thanks for the review, this is a tricky one

yes to a radical alternatives... this frustrates me ..Let me state the problem in very simplistic terms just so someone who has seen this before can say .. Oh that is easy.

I want different features to be present on (A) the release build and (B) the dev build.

Making geo-types optional is bad when it must always be included. I was constrained here ... there are long threads on stackoverflow about this - and meandering discussions amongst rustlang developers with lots of well meaning comments along the line of .. we can do better but no action. I hope I am just making a silly mistake.

To be clear, I'm just waiting a few days to solicit feedback from @georust/geo-publishers

The specific question is:

Since we already include approx by default in geo-types, are we ok with adding these implementations of approx traits on geo-types by default as well.

This looks like the feature we'd want: rust-lang/cargo#7916

This looks like the feature we'd want: rust-lang/cargo#7916

And if so, it looks like this feature should stabilize soon: rust-lang/cargo#8997

So it seems like we have a few options:

Wait to merge this until Tracking issue for -Z features=dev_dep rust-lang/cargo#7916 lands on stable Rust

Make geo-types optional, which is what this pull request currently does. When Tracking issue for -Z features=dev_dep rust-lang/cargo#7916 lands, we can make geo-types required again

Keep geo-types required, but always include the relative_eq feature. Even on non-test builds. When Tracking issue for -Z features=dev_dep rust-lang/cargo#7916 lands, we can turn on relative_eq only in dev-dependencies

I'm fine with any of these options, with a slight preference for option 3. It's unfortunate we introduce a new dependency for everyone, but it'll hopefully be temporary.

If we go with options 2 or 3, we should add a comment or open a GitHub Issue saying we can improve the situation when rust-lang/cargo#7916 lands.

I'm working on a slightly different proposal... hopefully I'll have something to share in 20min.

please see martinfrances107#1

What do you think of this solution @frewsxcv (et al)?

Currently in master:

geo-types dependency on approx is not optional

the methods in geo_types::private_utils are unconditionally built, but only used within geo-types if the rstar feature is enabled.

After this PR:

geo-types dependency on approx is optional

These new implementations for RelativeEq and AbsDiffEq are enabled with the now optional approx feature

There is "public" API breakage, in that methods in geo_types::private_utils are now skipped unless the rstar feature which requires them is enabled (geo builds geo-types w/ rstar feature, so this won't affect geo).

So technically, if some other crate is using the methods in private_utils, and not enabling the rstar feature, this would break for them. Even though this technically breaks a public API, I think it's OK to treat this as a minor change since the changes are in a module with the word "private" in its name. WDYT?

SGTM! Great solution

michaelkirk · 2020-12-29T18:16:19Z

geo/src/algorithm/rotate.rs

-        let line1 = Line::from([(1., 0.9999999999999999), (-1., 1.)]);
-        assert_eq!(line0.rotate(90.), line1);
+        let line1 = Line::from([(1., 1.), (-1., 1.)]);
+        assert_relative_eq!(line0.rotate(90.0), line1);


michaelkirk · 2020-12-29T18:16:41Z

geo/src/algorithm/rotate.rs

    }
    #[test]
    fn test_rotate_line_around_point() {
        let line0 = Line::new(Point::new(0., 0.), Point::new(0., 2.));
-        let line1 = Line::new(
-            Point::new(0., 0.),
-            Point::new(-2., 0.00000000000000012246467991473532),


🎉 yay! good riddance!

michaelkirk · 2020-12-29T18:26:07Z

And thank you for all the good docs and tests!

…d comments.

martinfrances107 · 2020-12-30T11:34:22Z

Thanks for the carefully review again... I have fixed all my silly mistakes.

The major problem concerning how to pull in different features into different build environment persists.

Rename `relative_eq` feature to `approx` and make optional

michaelkirk

bors r+

Ok, everything seems to be in order! Thanks for working through the lengthy review process with us @martinfrances107. I'm pretty happy with where it ended up. Thanks for your contribution!

bors · 2021-01-04T18:28:18Z

Build succeeded:

628: approx for all Geometry types r=frewsxcv a=michaelkirk - [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. --- Follow up to #567 which implemented for Coord, Point, LineString, and a few others, this PR implements it for the remaining types, and `Geometry` itself. FYI this is a precursor to an integration I'm working on with some of the JTS test suite. Co-authored-by: Michael Kirk <[email protected]>

martinfrances107 mentioned this pull request Dec 11, 2020

Tests are brittle. Use the concept of in_delta #565

Closed

martinfrances107 commented Dec 11, 2020

View reviewed changes

geo-types/src/point.rs Outdated Show resolved Hide resolved

michaelkirk reviewed Dec 15, 2020

View reviewed changes

martinfrances107 force-pushed the relative_eq branch from 5c9f43b to e875d03 Compare December 22, 2020 13:07

martinfrances107 added 15 commits December 24, 2020 11:44

Added num_coords() method to MultiPoint. To Point, Line, LineString a…

946397d

…nd MultiPoint test helpers and associated tests were added to assert that two structs were relatively equal. That is identical within the limits of numerical rounding accuracy.

Removed debug.

65f6100

Removed commented out code.

4ab3599

Removed commented out code.

daf5570

line_string.rs abs_diff_eq: simplify by using .all()

719c32c

multi_point.rs: Simplify using .all()

6c23693

multi_point.rs abs_diff_eq() now handles unequal sizes

3eaec49

point.rs Simplify RelativeEq for Point

1dca719

lines.rs Simplify "AbsDiffEq for Line<T>" to operate of coords not po…

edb4ff1

…ints.

line_string.rs - abs_diff_eq() Now rejects line_strings with unequal …

5aa011c

…lengths. (with tests)

Removed type constraint on RelativeEq and AbsDiffEq that the type mus…

efb112a

…t be Float. Also AbsDiffEq for Point now uses AbsDiffEq in Coords.

ran cargo fmt

8ba2b0b

Added new feature relative_eq

26a6a2d

After rebase, drop the use of the deprecated num_coords() method.

c21a552

Fixed dev-dependency issue.

bfb2746

martinfrances107 force-pushed the relative_eq branch from 89eaeb1 to bfb2746 Compare December 24, 2020 11:50

michaelkirk requested changes Dec 29, 2020

View reviewed changes

Minor Tidy: Removed multipoint.rs num_coords(). Fixed doctest code an…

d347afd

…d comments.

Rename relative_eq feature to approx and make optional

89d4742

michaelkirk mentioned this pull request Jan 1, 2021

Rename relative_eq feature to approx and make optional martinfrances107/geo#1

Merged

changelog

fde7395

Merge pull request #1 from georust/mkirk/approx-feature-juggling

e90ee64

Rename `relative_eq` feature to `approx` and make optional

michaelkirk approved these changes Jan 4, 2021

View reviewed changes

bors bot merged commit 7dbc853 into georust:master Jan 4, 2021

martinfrances107 deleted the relative_eq branch January 5, 2021 09:48

michaelkirk mentioned this pull request Jan 19, 2021

Add GeodesicIntermediate algorithm #608

Merged

2 tasks

michaelkirk mentioned this pull request Feb 20, 2021

approx for all Geometry types #628

Merged

2 tasks

relative_eq() #567

relative_eq() #567

Conversation

martinfrances107 commented Dec 11, 2020 • edited Loading

michaelkirk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk Dec 29, 2020 • edited Loading

Choose a reason for hiding this comment

michaelkirk Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

martinfrances107 commented Dec 16, 2020

michaelkirk commented Dec 16, 2020

martinfrances107 commented Dec 24, 2020 • edited Loading

michaelkirk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk Dec 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk commented Dec 29, 2020

martinfrances107 commented Dec 30, 2020

michaelkirk left a comment • edited Loading

Choose a reason for hiding this comment

bors bot commented Jan 4, 2021

martinfrances107 commented Dec 11, 2020 •

edited

Loading

michaelkirk Dec 16, 2020 •

edited

Loading

michaelkirk Dec 29, 2020 •

edited

Loading

michaelkirk Dec 15, 2020 •

edited

Loading

martinfrances107 commented Dec 24, 2020 •

edited

Loading

michaelkirk left a comment •

edited

Loading

michaelkirk Dec 29, 2020 •

edited

Loading

michaelkirk left a comment •

edited

Loading