Vector2DOps Trait - Proposal #1025

thehappycheese · 2023-06-27T17:05:29Z

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.

Proposal to define Vector2DOps trait and implement it for the Coord struct.
Intention is to facilitate general purpose linear algebra operations such as 'dot product' etc all in one place.

Most of these functions are are already implemented in various places throughout the crate or sometimes hard-coded in place. This would bring them all together.

For example

wedge_product( aka cross/perp/exterior product) geotypes::Line::determinant,
- geotypes::Point::cross_prod (which takes three arguments for some reason) etc.
- Also used in the default implementation of Kernel orient2d, although this serves a different purpose as it returns an enum
dot_product geotypes::Point::dot,
magnitude in Line::line_euclidean_length
magnitude_squared hard coded ( here)
normalize ... interestingly it is hard to find an example of this operation in the existing crate. Possibly because it is generally best avoided due to floating point instability.

For more discussion about motivation for this proposal, see this comment on PR #935 .

Possibly the timing of this proposal is weird, I am sure that the geotraits::CoordTrait will affect how this should be implemented. I suspect it may allow the Vector2DOps to define default implementations?

Note:

Currently Vector2DOps isn't used anywhere so it will trigger dead code warnings.
For an example that illustrates how I imagine it would be used in the OffsetCurve trait, see this snippet from this PR Implement Offset Algorithm #935
- Note that PR Implement Offset Algorithm #935 is a separate, older branch, in that branch I was calling it VectorExtensions. In this PR I renamed it to Vector2DOps so that it is more similar to the existing AffineOps

Many thanks for your time and any feedback you can provide

michaelkirk

Sorry for the delay - I thought I submitted a review a long time ago, but it seems like it didn't go through (or I'm hallucinating).

michaelkirk · 2023-07-11T22:58:20Z

geo/src/algorithm/mod.rs

+/// # Vector Operations for 2D coordinates
+/// 
+mod vector_ops;
+pub(crate) use vector_ops::Vector2DOps;


Is there a reason for this not to be public?

Hi @michaelkirk no stress about the delay, thankyou for your time :)

No problems making it public. I was thinking of this as an internal tool, but some crate users may find it useful.

One benefit of making it private is to reduce the complexity of the API exposed to the user. For example, a user might want to find the length of a Line, but miss the EuclideanLength trait in the docs, and mistakenly assume they should use Vector2DOps::magnitude:

use geo::algorithim::Vector2DOps; my_line.delta().magnitude()

instead of

use geo::EuclideanLength; my_line.euclidean_length()

On the other hand making it private means 'cargo doc' may hide it away and make it harder for other crate contributors to use it. The user also misses out on some useful functionality.

I think changing it to be public is probably the way to go, and follows the established pattern in algorithm/mod.rs?
Maybe it can just not be re-exported at the top level or something?

👍 I'm in favor of making them public and treating them like all our other algorithm modules. They seem like generally useful methods.

Ok sounds like a plan to me :)

michaelkirk

Is there a reason that this is a draft, or would you be happy to move forward with merging it?

michaelkirk · 2023-07-12T17:28:27Z

geo/src/algorithm/vector_ops.rs

+where
+    Self: Sized,
+{
+    type NumericType: CoordNum + Send + Sync;


Suggested change

type NumericType: CoordNum + Send + Sync;

type Scalar: CoordNum + Send + Sync;

We have, I think, similar associated types elsewhere in the codebase which we call Scalar - can we use that naming here too to stay conventional, or is that not accurate in this case?

Changed to Scalar, that is the correct terminology 👍

michaelkirk · 2023-07-12T17:29:44Z

geo/src/algorithm/mod.rs

+/// # Vector Operations for 2D coordinates
+/// 
+mod vector_ops;
+pub(crate) use vector_ops::Vector2DOps;


👍 I'm in favor of making them public and treating them like all our other algorithm modules. They seem like generally useful methods.

kylebarron · 2023-07-12T17:37:13Z

Possibly the timing of this proposal is weird, I am sure that the geotraits::CoordTrait will affect how this should be implemented.

I think coordinate traits will take a while to get stabilized, so I wouldn't put too much thought in that work for your PR

michaelkirk

Other than some final nits, this LGTM.

michaelkirk · 2023-07-13T19:06:17Z

geo/src/algorithm/vector_ops.rs

+    // crate dependencies
+    use crate::coord;
+
+    // private imports


Suggested change

// private imports

nit: comment out of date since this is no longer private

michaelkirk · 2023-07-13T19:09:53Z

geo/src/algorithm/vector_ops.rs

+        assert_eq!(a.try_normalize(f64_minimum_threshold), None);
+
+        // Large vector Pass;
+        // Note: this fails because the magnitude calculation overflows to infinity


This comment seems wrong - this example passes, right?

ah yes correct that one is meant to pass... looks like I botched a copy paste

michaelkirk · 2023-07-13T19:11:24Z

geo/src/algorithm/vector_ops.rs

+
+        // Large Vector Fail;
+        // Note: This test uses the unstable float_next_up_down feature
+        // let a = coord! { x: f64::sqrt(f64::MAX/2.0), y: f64::next_up(f64::sqrt(f64::MAX/2.0)) };


Not a big deal, but if you wanted to show it failing how about something simpler like:

Suggested change

// let a = coord! { x: f64::sqrt(f64::MAX/2.0), y: f64::next_up(f64::sqrt(f64::MAX/2.0)) };

let a = coord! { x: f64::sqrt(f64::MAX/2.0 + 1.0), y: f64::next_up(f64::sqrt(f64::MAX/2.0 + 1.0)) };

Hi @michaelkirk; I think you meant to remove the nightly-only f64::next_up from your suggestion and use + 1.0 instead?

The trouble is that for very large floats we don't know the next largest representable number so we can get the unexpected result;

assert_eq!(f64::MAX/2.0 + 1.0, f64::MAX/2.0); // passes unexpectedly

I have just spotted that there is already a dependency on the float_next_after crate. I will use that instead.

I just had a bit of a noodle and it turns out the difference between f64::MAX/2.0 and the next highest representable number is something like 9.9e291 which is kinda confusing to think about.

println!("{:e}", f64::MAX / 2.0); println!("{:e}", (f64::MAX/2.0).next_up()); println!("{:e}", (f64::MAX/2.0).next_up() - f64::MAX / 2.0); // 8.988465674311579e307 // 8.98846567431158e307 // 9.9792015476736e291

Now I am wondering about the value of some of the asserts in test_normalize(); Perhaps some are really testing the f64 implementation more than the try_normalize and should be removed?

Maybe the best thing is to try add some notes to the documentation rather than try to check everything in the tests? Just thinking out loud here; There are basically 2 ways it can go wrong;

The user has some massive vector (plausibly arising from some algorithm that sums many vectors) where the sum of the squares overflows before the square root. This situation could be mitigated by detecting large vectors and scaling down before doing the calculation.

The user normalizes a really small vector and has chosen a bad value for the threshold causing a divide by zero or overflow (both result in f64::INFINITY).

Actually on further investigation coord!{x:0,y:1.0/f64::MAX}.try_normalize(0.0) returns None because coord!{x:0,y:1.0/f64::MAX}.magnitude() underflows to 0.0 😟. This is the same problem in reverse, and it means there are many valid small vectors that will return None despite the magnitude being greater than the user's chosen threshold. This situation could be mitigated by first scaling up the vector before normalizing.

Ok after that 45 min decent into madness I have an idea; it is easier to just check if the magnitude and the final result are finite after the calculation. There is no point guarding against invalid float operations since they do not panic anyway. Also this new version is less annoying to use because you don't need to provide a minimum threshold;

fn try_normalize(self) -> Option<Self> { let magnitude = self.magnitude(); let result = self / magnitude; if result.is_finite() && magnitude.is_finite() { Some(result) } else { None } } fn is_finite(self) -> bool { self.x.is_finite() && self.y.is_finite() }

I have also improved the doc comment in my next commit.

Thanks for reading through my unreasonable suggestion and coming up with something better!

michaelkirk

LGTM, but I'll give it a couple more days before merging in case anyone else wanted to weigh in.

michaelkirk · 2023-07-26T17:35:59Z

geo/src/algorithm/vector_ops.rs

+
+        // Large Vector Fail;
+        // Note: This test uses the unstable float_next_up_down feature
+        // let a = coord! { x: f64::sqrt(f64::MAX/2.0), y: f64::next_up(f64::sqrt(f64::MAX/2.0)) };


Thanks for reading through my unreasonable suggestion and coming up with something better!

michaelkirk · 2023-07-28T20:30:29Z

bors r+

1025: Vector2DOps Trait - Proposal r=michaelkirk a=thehappycheese - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Proposal to define `Vector2DOps` trait and implement it for the `Coord` struct. Intention is to facilitate general purpose linear algebra operations such as 'dot product' etc all in one place. Most of these functions are are already implemented in various places throughout the crate or sometimes hard-coded in place. This would bring them all together. For example - `wedge_product`( aka cross/perp/exterior product) `geotypes::Line::determinant`, - `geotypes::Point::cross_prod` (which takes three arguments for some reason) etc. - Also used in the default implementation of Kernel orient2d, although this serves a different purpose as it returns an enum - `dot_product` `geotypes::Point::dot`, - `magnitude` in `Line::line_euclidean_length` - `magnitude_squared` hard coded ( [here](https://github.com/thehappycheese/geo/blob/9b477fc7b24fcb8731bd5e01d559b613e79ada0c/geo/src/algorithm/line_locate_point.rs#L61C9-L61C29)) - `normalize` ... interestingly it is hard to find an example of this operation in the existing crate. Possibly because it is generally best avoided due to floating point instability. For more discussion about motivation for this proposal, see [this comment](#935 (comment)) on PR #935 . Possibly the timing of this proposal is weird, I am sure that the `geotraits::CoordTrait` will affect how this should be implemented. I suspect it may allow the `Vector2DOps` to define default implementations? Note: - Currently `Vector2DOps` isn't used anywhere so it will trigger dead code warnings. - For an example that illustrates how I imagine it would be used in the OffsetCurve trait, see [this snippet](https://github.com/thehappycheese/geo/blob/d95420f1f8d6b0d2dd1e6d803fe3d3e2a2b3dd13/geo/src/algorithm/offset_curve/offset_line_raw.rs#L46C1-L51C1) from this PR #935 - Note that PR #935 is a separate, older branch, in that branch I was calling it `VectorExtensions`. In this PR I renamed it to `Vector2DOps` so that it is more similar to the existing `AffineOps` Many thanks for your time and any feedback you can provide Co-authored-by: thehappycheese <[email protected]>

bors · 2023-07-28T20:35:21Z

Build failed:

ci result

michaelkirk · 2023-07-28T23:09:34Z

Can you please run cargo fmt and push back up? It doesn't like your CRLF line terminators and a couple other things.

thehappycheese · 2023-07-30T05:15:58Z

Can you please run cargo fmt and push back up? It doesn't like your CRLF line terminators and a couple other things.

Hi @michaelkirk, appologies for that, I ran cargo fmt again and I think it is fixed now in 12155aa

I'm pretty sure it has been committed using LF and not CRLF.
The git docs say 'You can tell Git to convert CRLF to LF on commit but not the other way around by setting core.autocrlf to input' so i have done that. I am surprised github has no built-in way to verify if it worked.

urschrei · 2023-07-30T07:19:41Z

Bors try

bors · 2023-07-30T07:25:27Z

try

Build failed:

ci result

urschrei · 2023-07-30T10:15:45Z

This is a spurious Clippy lint failure imo, because the suggested change:

--- a/geo/src/algorithm/vector_ops.rs
+++ b/geo/src/algorithm/vector_ops.rs
@@ -111,7 +111,7 @@ where
     fn try_normalize(self) -> Option<Self>;
 
     /// Returns true if both the x and y components are finite
-    fn is_finite(self) -> bool;
+    fn is_finite(&self) -> bool;
 }
 
 impl<T> Vector2DOps for Coord<T>
@@ -165,7 +165,7 @@ where
         }
     }
 
-    fn is_finite(self) -> bool {
+    fn is_finite(self: &geo_types::Coord<T>) -> bool {
         self.x.is_finite() && self.y.is_finite()
     }
 }

Introduces an unnecessary borrow of Coord (a Copy type).

If you annotate the trait fn instead:

--- a/geo/src/algorithm/vector_ops.rs
+++ b/geo/src/algorithm/vector_ops.rs
@@ -111,6 +111,7 @@ where
     fn try_normalize(self) -> Option<Self>;
 
     /// Returns true if both the x and y components are finite
+    #[allow(clippy::wrong_self_convention)]
     fn is_finite(self) -> bool;
 }

All should be well.

urschrei · 2023-07-31T10:45:12Z

bors try

bors · 2023-07-31T10:51:03Z

try

Build failed:

ci result

thehappycheese · 2023-07-31T10:53:26Z

I am confused... There should be a 12th commit where I added the annotation, but it is not showing up in this PR. Is it because it happened while bors was doing something?

urschrei · 2023-07-31T10:55:47Z

I'm confused too. I don't think it was bors, and it's…somewhere:

006a48c

urschrei · 2023-07-31T10:57:22Z

Can you pull your remote branch and then push again, just to be sure? I can see the commit on your remote, but it's now not in the PR for some reason.

thehappycheese · 2023-07-31T11:00:04Z

I cant find a way to trigger it manually. I tried the open pull request button but it just navigates to this existing PR. I can try commit some minor change to a comment or something and see if that triggers it to get included? Maybe I need to rebase it or something? It still says it can be merged tho.

urschrei · 2023-07-31T11:01:42Z

Yeah, try an inconsequential commit.

thehappycheese · 2023-07-31T11:03:42Z

Ah I clicked "Sync Fork" and that created a 13th commit... so far that hasn't been needed. I wonder why it was needed this time

urschrei · 2023-07-31T11:05:01Z

bors r=michaelkirk

1025: Vector2DOps Trait - Proposal r=michaelkirk a=thehappycheese - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Proposal to define `Vector2DOps` trait and implement it for the `Coord` struct. Intention is to facilitate general purpose linear algebra operations such as 'dot product' etc all in one place. Most of these functions are are already implemented in various places throughout the crate or sometimes hard-coded in place. This would bring them all together. For example - `wedge_product`( aka cross/perp/exterior product) `geotypes::Line::determinant`, - `geotypes::Point::cross_prod` (which takes three arguments for some reason) etc. - Also used in the default implementation of Kernel orient2d, although this serves a different purpose as it returns an enum - `dot_product` `geotypes::Point::dot`, - `magnitude` in `Line::line_euclidean_length` - `magnitude_squared` hard coded ( [here](https://github.com/thehappycheese/geo/blob/9b477fc7b24fcb8731bd5e01d559b613e79ada0c/geo/src/algorithm/line_locate_point.rs#L61C9-L61C29)) - `normalize` ... interestingly it is hard to find an example of this operation in the existing crate. Possibly because it is generally best avoided due to floating point instability. For more discussion about motivation for this proposal, see [this comment](#935 (comment)) on PR #935 . Possibly the timing of this proposal is weird, I am sure that the `geotraits::CoordTrait` will affect how this should be implemented. I suspect it may allow the `Vector2DOps` to define default implementations? Note: - Currently `Vector2DOps` isn't used anywhere so it will trigger dead code warnings. - For an example that illustrates how I imagine it would be used in the OffsetCurve trait, see [this snippet](https://github.com/thehappycheese/geo/blob/d95420f1f8d6b0d2dd1e6d803fe3d3e2a2b3dd13/geo/src/algorithm/offset_curve/offset_line_raw.rs#L46C1-L51C1) from this PR #935 - Note that PR #935 is a separate, older branch, in that branch I was calling it `VectorExtensions`. In this PR I renamed it to `Vector2DOps` so that it is more similar to the existing `AffineOps` Many thanks for your time and any feedback you can provide Co-authored-by: thehappycheese <[email protected]>

bors · 2023-07-31T11:10:21Z

Build failed:

ci result

urschrei · 2023-07-31T11:12:07Z

Now it's timing out. This is hilarious.

urschrei · 2023-07-31T11:12:14Z

bors retry

1025: Vector2DOps Trait - Proposal r=michaelkirk a=thehappycheese - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Proposal to define `Vector2DOps` trait and implement it for the `Coord` struct. Intention is to facilitate general purpose linear algebra operations such as 'dot product' etc all in one place. Most of these functions are are already implemented in various places throughout the crate or sometimes hard-coded in place. This would bring them all together. For example - `wedge_product`( aka cross/perp/exterior product) `geotypes::Line::determinant`, - `geotypes::Point::cross_prod` (which takes three arguments for some reason) etc. - Also used in the default implementation of Kernel orient2d, although this serves a different purpose as it returns an enum - `dot_product` `geotypes::Point::dot`, - `magnitude` in `Line::line_euclidean_length` - `magnitude_squared` hard coded ( [here](https://github.com/thehappycheese/geo/blob/9b477fc7b24fcb8731bd5e01d559b613e79ada0c/geo/src/algorithm/line_locate_point.rs#L61C9-L61C29)) - `normalize` ... interestingly it is hard to find an example of this operation in the existing crate. Possibly because it is generally best avoided due to floating point instability. For more discussion about motivation for this proposal, see [this comment](#935 (comment)) on PR #935 . Possibly the timing of this proposal is weird, I am sure that the `geotraits::CoordTrait` will affect how this should be implemented. I suspect it may allow the `Vector2DOps` to define default implementations? Note: - Currently `Vector2DOps` isn't used anywhere so it will trigger dead code warnings. - For an example that illustrates how I imagine it would be used in the OffsetCurve trait, see [this snippet](https://github.com/thehappycheese/geo/blob/d95420f1f8d6b0d2dd1e6d803fe3d3e2a2b3dd13/geo/src/algorithm/offset_curve/offset_line_raw.rs#L46C1-L51C1) from this PR #935 - Note that PR #935 is a separate, older branch, in that branch I was calling it `VectorExtensions`. In this PR I renamed it to `Vector2DOps` so that it is more similar to the existing `AffineOps` Many thanks for your time and any feedback you can provide Co-authored-by: thehappycheese <[email protected]>

bors · 2023-07-31T11:17:06Z

This PR was included in a batch that successfully built, but then failed to merge into main. It will not be retried.

Additional information:

Response status code: 422
{"message":"Changes must be made through the merge queue","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

urschrei · 2023-07-31T11:21:13Z

I though we turned off the merge queue, but apparently not, and it's now conflicting with bors. The PR passes so I'm merging using the queue, then going for a little walk.

thehappycheese · 2023-07-31T11:31:34Z

Hooray, thanks for all the help to get it merged, I'm looking forward to working on the next part of the other offset curve PR 😄

michaelkirk · 2023-07-31T17:58:25Z

I hadn't turned it off - I just never got it to work.

I did, just now, turn it off though. For the record it's under Settings > Branches (branch protection rules) > Edit

thehappycheese added 3 commits June 28, 2023 00:37

add trait VectorOps & impl for Coord

5d0e037

fix test for try_normalize

7a9a6d9

fix format

b6a2227

thehappycheese changed the title ~~VectorOps proposal~~ Vector2DOps Trait - Proposal Jun 27, 2023

michaelkirk reviewed Jul 11, 2023

View reviewed changes

michaelkirk reviewed Jul 12, 2023

View reviewed changes

thehappycheese added 6 commits July 13, 2023 20:17

Rename associated type NumericType to Scalar

923b8f6

make Vector2DOps public

0ba00f0

remove commentary intended for reviewers

fe9df2a

improve tests for try_normalize

8763ef1

tidy doc comment for try_normalize

1c132ae

add Vector2DOps to CHANGES.md

5706059

thehappycheese marked this pull request as ready for review July 13, 2023 16:43

michaelkirk approved these changes Jul 14, 2023

View reviewed changes

removed threshold from Vector2DOps::try_normalize

43d2fef

michaelkirk approved these changes Jul 26, 2023

View reviewed changes

cargo fmt & check line endings are LF

12155aa

bors bot added a commit that referenced this pull request Jul 30, 2023

Try #1025:

a1b9f1f

add annotation to disable bad clippy lint

006a48c

bors bot added a commit that referenced this pull request Jul 31, 2023

Try #1025:

218a4a8

Merge branch 'georust:main' into vector-ops-proposal

10b7f08

urschrei added this pull request to the merge queue Jul 31, 2023

Merged via the queue into georust:main with commit d87fc54 Jul 31, 2023

	type NumericType: CoordNum + Send + Sync;
	type Scalar: CoordNum + Send + Sync;

	// let a = coord! { x: f64::sqrt(f64::MAX/2.0), y: f64::next_up(f64::sqrt(f64::MAX/2.0)) };
	let a = coord! { x: f64::sqrt(f64::MAX/2.0 + 1.0), y: f64::next_up(f64::sqrt(f64::MAX/2.0 + 1.0)) };

Vector2DOps Trait - Proposal #1025

Vector2DOps Trait - Proposal #1025

Conversation

thehappycheese commented Jun 27, 2023 • edited Loading

michaelkirk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehappycheese Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

kylebarron commented Jul 12, 2023

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

thehappycheese Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk commented Jul 28, 2023

bors bot commented Jul 28, 2023

michaelkirk commented Jul 28, 2023

thehappycheese commented Jul 30, 2023

urschrei commented Jul 30, 2023

bors bot commented Jul 30, 2023

try

urschrei commented Jul 30, 2023

urschrei commented Jul 31, 2023

bors bot commented Jul 31, 2023

try

thehappycheese commented Jul 31, 2023

urschrei commented Jul 31, 2023

urschrei commented Jul 31, 2023

thehappycheese commented Jul 31, 2023

urschrei commented Jul 31, 2023

thehappycheese commented Jul 31, 2023

urschrei commented Jul 31, 2023

bors bot commented Jul 31, 2023

urschrei commented Jul 31, 2023

urschrei commented Jul 31, 2023

bors bot commented Jul 31, 2023

urschrei commented Jul 31, 2023

thehappycheese commented Jul 31, 2023

michaelkirk commented Jul 31, 2023

thehappycheese commented Jun 27, 2023 •

edited

Loading

thehappycheese Jul 12, 2023 •

edited

Loading

thehappycheese Jul 26, 2023 •

edited

Loading