Vector2DOps Trait - Proposal

[thehappycheese]

• [x] I agree to follow the project's code of conduct.

• [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)
• 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 #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

[kylebarron]

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]

bors r+

[bors[bot]]

Build failed:

• ci result

[michaelkirk]

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

[thehappycheese]

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]

Bors try

[bors[bot]]

try

Build failed:

• ci result

[urschrei]

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]

bors try

[bors[bot]]

try

Build failed:

• ci result

[thehappycheese]

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]

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

https://github.com/georust/geo/commit/006a48c1be8430a74ad916674790cb9cc8ae2f1d

[urschrei]

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]

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]

Yeah, try an inconsequential commit.

[thehappycheese]

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]

bors r=michaelkirk

[bors[bot]]

Build failed:

• ci result

[urschrei]

Now it's timing out. This is hilarious.

[urschrei]

bors retry

[bors[bot]]

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]

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]

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]

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