Support generic numeric type for geometries #30

zarch · 2016-05-18T07:46:33Z

Hi, here a revised version to add generics to geometries (see previous pull request #25).
I create a separate branch called: generics to make it easier to keep it sync with the official master.
I've modified the code to remove most of the Neg<Output = T> as suggested by @jdroenner , I was not able to remove Copy without compilation errors so it is still there.

As suggested by @jdroenner we can just pass the reference, in this way we can remove most of the Copy, but it required to add some clone() to pass the tests, here you can see the difference:

diff --git a/src/types.rs b/src/types.rs
index 78ba3f4..dae8244 100644
--- a/src/types.rs
+++ b/src/types.rs
@@ -4,16 +4,16 @@ use std::ops::Sub;

 use num::Num;

-#[derive(PartialEq, Clone, Copy, Debug)]
+#[derive(PartialEq, Clone, Debug)]
 pub struct Coordinate<T>
-    where T: Num + Copy
+    where T: Num
 {
     pub x: T,
     pub y: T,
 }

-#[derive(PartialEq, Clone, Copy, Debug)]
-pub struct Point<T> (pub Coordinate<T>) where T: Num + Copy;
+#[derive(PartialEq, Clone, Debug)]
+pub struct Point<T> (pub Coordinate<T>) where T: Num;

 impl<T> Point<T>
     where T: Num + Copy
@@ -25,8 +25,8 @@ impl<T> Point<T>
     ///
     /// let p = Point::new(1.234, 2.345);
     ///
-    /// assert_eq!(p.x(), 1.234);
-    /// assert_eq!(p.y(), 2.345);
+    /// assert_eq!(*p.x(), 1.234);
+    /// assert_eq!(*p.y(), 2.345);
     /// ```
     pub fn new(x: T, y: T) -> Point<T> {
         Point(Coordinate {
@@ -42,10 +42,10 @@ impl<T> Point<T>
     ///
     /// let p = Point::new(1.234, 2.345);
     ///
-    /// assert_eq!(p.x(), 1.234);
+    /// assert_eq!(*p.x(), 1.234);
     /// ```
-    pub fn x(&self) -> T {
-        self.0.x
+    pub fn x(&self) -> &T {
+        &self.0.x
     }

     /// Sets the x/horizontal component of the point.
@@ -56,7 +56,7 @@ impl<T> Point<T>
     /// let mut p = Point::new(1.234, 2.345);
     /// p.set_x(9.876);
     ///
-    /// assert_eq!(p.x(), 9.876);
+    /// assert_eq!(*p.x(), 9.876);
     /// ```
     pub fn set_x(&mut self, x: T) -> &mut Point<T> {
         self.0.x = x;
@@ -70,10 +70,10 @@ impl<T> Point<T>
     ///
     /// let p = Point::new(1.234, 2.345);
     ///
-    /// assert_eq!(p.y(), 2.345);
+    /// assert_eq!(*p.y(), 2.345);
     /// ```
-    pub fn y(&self) -> T {
-        self.0.y
+    pub fn y(&self) -> &T {
+        &self.0.y
     }

     /// Sets the y/vertical component of the point.
@@ -84,7 +84,7 @@ impl<T> Point<T>
     /// let mut p = Point::new(1.234, 2.345);
     /// p.set_y(9.876);
     ///
-    /// assert_eq!(p.y(), 9.876);
+    /// assert_eq!(*p.y(), 9.876);
     /// ```
     pub fn set_y(&mut self, y: T) -> &mut Point<T> {
         self.0.y = y;
@@ -98,9 +98,9 @@ impl<T> Point<T>
     ///
     /// let p = Point::new(1.234, 2.345);
     ///
-    /// assert_eq!(p.lng(), 1.234);
+    /// assert_eq!(*p.lng(), 1.234);
     /// ```
-    pub fn lng(&self) -> T {
+    pub fn lng(&self) -> &T {
         self.x()
     }

@@ -112,7 +112,7 @@ impl<T> Point<T>
     /// let mut p = Point::new(1.234, 2.345);
     /// p.set_lng(9.876);
     ///
-    /// assert_eq!(p.lng(), 9.876);
+    /// assert_eq!(*p.lng(), 9.876);
     /// ```
     pub fn set_lng(&mut self, lng: T) -> &mut Point<T> {
         self.set_x(lng)
@@ -125,9 +125,9 @@ impl<T> Point<T>
     ///
     /// let p = Point::new(1.234, 2.345);
     ///
-    /// assert_eq!(p.lat(), 2.345);
+    /// assert_eq!(*p.lat(), 2.345);
     /// ```
-    pub fn lat(&self) -> T {
+    pub fn lat(&self) -> &T {
         self.y()
     }

@@ -139,7 +139,7 @@ impl<T> Point<T>
     /// let mut p = Point::new(1.234, 2.345);
     /// p.set_lat(9.876);
     ///
-    /// assert_eq!(p.lat(), 9.876);
+    /// assert_eq!(*p.lat(), 9.876);
     /// ```
     pub fn set_lat(&mut self, lat: T) -> &mut Point<T> {
         self.set_y(lat)
@@ -157,7 +157,7 @@ impl<T> Point<T>
     /// assert_eq!(dot, 5.25);
     /// ```
     pub fn dot(&self, point: &Point<T>) -> T {
-        self.x() * point.x() + self.y() * point.y()
+        *self.x() * *point.x() + *self.y() * *point.y()
     }
 }

@@ -173,11 +173,11 @@ impl<T> Neg for Point<T>
     ///
     /// let p = -Point::new(-1.25, 2.5);
     ///
-    /// assert_eq!(p.x(), 1.25);
-    /// assert_eq!(p.y(), -2.5);
+    /// assert_eq!(*p.x(), 1.25);
+    /// assert_eq!(*p.y(), -2.5);
     /// ```
     fn neg(self) -> Point<T> {
-        Point::new(-self.x(), -self.y())
+        Point::new(-*self.x(), -*self.y())
     }
 }

@@ -193,11 +193,11 @@ impl<T> Add for Point<T>
     ///
     /// let p = Point::new(1.25, 2.5) + Point::new(1.5, 2.5);
     ///
-    /// assert_eq!(p.x(), 2.75);
-    /// assert_eq!(p.y(), 5.0);
+    /// assert_eq!(*p.x(), 2.75);
+    /// assert_eq!(*p.y(), 5.0);
     /// ```
     fn add(self, rhs: Point<T>) -> Point<T> {
-        Point::new(self.x() + rhs.x(), self.y() + rhs.y())
+        Point::new(*self.x() + *rhs.x(), *self.y() + *rhs.y())
     }
 }

@@ -213,35 +213,35 @@ impl<T> Sub for Point<T>
     ///
     /// let p = Point::new(1.25, 3.0) - Point::new(1.5, 2.5);
     ///
-    /// assert_eq!(p.x(), -0.25);
-    /// assert_eq!(p.y(), 0.5);
+    /// assert_eq!(*p.x(), -0.25);
+    /// assert_eq!(*p.y(), 0.5);
     /// ```
     fn sub(self, rhs: Point<T>) -> Point<T> {
-        Point::new(self.x() - rhs.x(), self.y() - rhs.y())
+        Point::new(*self.x() - *rhs.x(), *self.y() - *rhs.y())
     }
 }

 #[derive(PartialEq, Clone, Debug)]
-pub struct MultiPoint<T>(pub Vec<Point<T>>) where T: Num + Copy;
+pub struct MultiPoint<T>(pub Vec<Point<T>>) where T: Num;

 #[derive(PartialEq, Clone, Debug)]
-pub struct LineString<T>(pub Vec<Point<T>>) where T: Num + Copy;
+pub struct LineString<T>(pub Vec<Point<T>>) where T: Num;

 #[derive(PartialEq, Clone, Debug)]
-pub struct MultiLineString<T>(pub Vec<LineString<T>>) where T: Num + Copy;
+pub struct MultiLineString<T>(pub Vec<LineString<T>>) where T: Num;

 #[derive(PartialEq, Clone, Debug)]
-pub struct Polygon<T>(pub LineString<T>, pub Vec<LineString<T>>) where T: Num + Copy;
+pub struct Polygon<T>(pub LineString<T>, pub Vec<LineString<T>>) where T: Num;

 #[derive(PartialEq, Clone, Debug)]
-pub struct MultiPolygon<T>(pub Vec<Polygon<T>>) where T: Num + Copy;
+pub struct MultiPolygon<T>(pub Vec<Polygon<T>>) where T: Num;

 #[derive(PartialEq, Clone, Debug)]
-pub struct GeometryCollection<T>(pub Vec<Geometry<T>>) where T: Num + Copy;
+pub struct GeometryCollection<T>(pub Vec<Geometry<T>>) where T: Num;

 #[derive(PartialEq, Clone, Debug)]
 pub enum Geometry<T>
-    where T: Num + Copy
+    where T: Num
 {
     Point(Point<T>),
     LineString(LineString<T>),
@@ -263,9 +263,9 @@ mod test {
             y: 116.34
         };

-        let p = Point(c);
+        let p = Point(c.clone());

-        let Point(c2) = p;
+        let Point(c2) = p.clone();
         assert_eq!(c, c2);
         assert_eq!(c.x, c2.x);
         assert_eq!(c.y, c2.y);

Other ideas to improve this? should we pass the Copy or the reference?

frewsxcv · 2016-05-19T02:17:17Z

src/types.rs

@@ -163,13 +169,17 @@ impl Point {
    ///
    /// assert!(dist < 1e-1)
    /// ```
-    pub fn distance_to(&self, point: &Point) -> f64 {
-        ((self.x() - point.x()).powi(2) + (self.y() - point.y()).powi(2)).sqrt()
+    pub fn distance_to(&self, point: &Point<T>) -> f64 {


If instead of using num::Num everywhere, we used num::Float, we could make distance_to generic by utilizing num::Float::sqrt.

It also means you can remove the explicit Copy trait bounds specifying everywhere since Float is a subtype of Copy.

frewsxcv · 2016-05-19T02:27:39Z

I wouldn't be too worried about the Copy trait bound right now. In the future, we can look into removing it if it's desired, but for now, I think it's okay to have Copy.

frewsxcv · 2016-05-19T02:28:51Z

Looks great, awesome work!

In general, I think it'd be great if all the operations could be made generic. Then we could remove the ToPrimitive trait bound as well.

frewsxcv · 2016-05-19T02:30:20Z

src/algorithm/centroid.rs

+                let x1 : f64 = p1.x().to_f64().unwrap();
+                let x2 : f64 = p2.x().to_f64().unwrap();
+                let y1 : f64 = p1.y().to_f64().unwrap();
+                let y2 : f64 = p2.y().to_f64().unwrap();
                total_length += segment_len;


If you try to make this generic, the type checker might complain. You might need to make this into total_length = total_length + segment_len;

Kerollmops · 2016-07-12T20:46:06Z

Up ? 😄

zarch · 2016-07-13T11:44:56Z

Hi Kerosene2000,
actually I've started the merging between the master and this branch, but actually I'm new to rust, and I'm stuck to some compile errors that I don't know how to fix. I push the changes on the branch. Perhaps someone can help to fix them.

Kerollmops · 2016-07-13T14:27:51Z

Here is the pull request I make to @zarch zarch#1. This way we can continue using this thread to speak and don't create another one.

jdroenner · 2016-07-13T16:32:18Z

Hi @Kerosene2000 and @zarch,
maybe you could replace lon()/lat() with x()/y() in your pull requests :) Using both of them is a bit confusing.

Kerollmops · 2016-07-13T16:35:37Z

Done 😄

TeXitoi · 2016-07-13T16:38:09Z

src/algorithm/centroid.rs

-                sum_x += tmp * p.lng();
-                sum_y += tmp * p.lat();
+                sum_x = sum_x + tmp * p.lng();
+                sum_y = sum_y + tmp * p.lat();


You forgot this one ;-)

You have to look at here 😄
Until @zarch merge with its branch

My bad, didn't see the pull request on the pull request 👼

Wow, thank you...

zarch · 2016-07-14T04:42:13Z

can you explain ?

Sorry, I didn't notice it, I've just accept your PR...
But it seems fix to me right now, isn't it?

Kerollmops · 2016-07-14T09:22:22Z

I think so !

frewsxcv · 2016-07-16T03:09:12Z

This looks great! Upon a quick glance, it looks like everything has been made generic. Is there anything else to do here before final reviews + merging?

frewsxcv · 2016-07-16T20:45:11Z

src/algorithm/centroid.rs

-    fn centroid(&self) -> Option<Point> {
-        // TODO: consideration of inner polygons;
+    fn centroid(&self) -> Option<Point<T>> {
+         // TODO: consideration of inner polygons;


Nit: extra whitespace in front here

frewsxcv · 2016-07-16T20:53:23Z

I just did a review. Besides the commented out code, which can be removed, I don't see any other changes that need to be made. Anyone else have thoughts?

frewsxcv · 2016-07-16T21:40:23Z

src/algorithm/distance.rs

+        let (dx, dy) = (self.x() - p.x(), self.y() - p.y());
+        // (dx * dx + dy * dy)**(T::one()/(T::one() + T::one()))
+        // ((self.x() - p.x()).powi(2) + (self.y() - p.y()).powi(2)).sqrt()
+        (dx * dx + dy * dy).sqrt() // FIXME is it correct ??? (euclidian dist)


These comments can be removed. The logic seems fine to me.

frewsxcv · 2016-07-17T03:21:16Z

Thank you @zarch and @Kerosene2000 for your contributions here! I'm looking forward to these changes! 🎊

I thought about squashing, but:

I don't think the git history is too messy
I want both @zarch and @Kerosene2000 to get credit for their commits
There is some value in seeing the evolution of ideas

I'm going to merge now, but I won't plan on publishing a new version of rust-geo (version 0.2) until Sunday evening Eastern time just to give some time in-case anyone has additional concerns.

frewsxcv · 2016-07-18T01:25:48Z

Version 0.2.0 of geo-rust has been released which includes these changes.

@frewsxcv

Merge georust#30 a=@frewsxcv r=@frewsxcv ________________________________________________________________________ I'm the only contributor, so this should be a straightforward license change.

Zambelli Pietro added 6 commits May 13, 2016 07:41

Support generic numeric type for all structs Issue#25

74334ba

Keep Neg only where needed

eb9a0e5

Remove Neg from traits.rs

cdae4db

Merge branch 'master' into generics

5ff339c

Remove unused import 'use std::ops::Neg;'

02bdbd6

Add generics also to centroids

fccd1ce

frewsxcv reviewed May 19, 2016
View reviewed changes

Zambelli Pietro and others added 2 commits May 19, 2016 15:12

Move from num::Num to num::Float

ae6ce89

use num::One::one

953589e

Merging from master

2b42ff5

Working great I believe :)

cda06e2

little cleanups

a80c0e4

lng -> x, lat -> x

277340e

TeXitoi reviewed Jul 13, 2016
View reviewed changes

Merge pull request #1 from Kerosene2000/generics

faadfc8

Wow, thank you...

frewsxcv reviewed Jul 16, 2016
View reviewed changes

Remove an extra whitespace

5fe7db0

frewsxcv reviewed Jul 16, 2016
View reviewed changes

Zambelli Pietro added 2 commits July 17, 2016 00:02

Remove unnecessary comments

51547b8

Remove unnecessary comment on COORD_PRECISION

1eaa582

frewsxcv merged commit e23eaa6 into georust:master Jul 17, 2016

frewsxcv mentioned this pull request Jul 17, 2016

Generic numeric type for all structs #25

Closed

zarch deleted the generics branch July 21, 2016 05:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support generic numeric type for geometries #30

Support generic numeric type for geometries #30

zarch commented May 18, 2016

frewsxcv May 19, 2016

frewsxcv May 19, 2016

frewsxcv commented May 19, 2016

frewsxcv commented May 19, 2016

frewsxcv May 19, 2016

Kerollmops commented Jul 12, 2016

zarch commented Jul 13, 2016

Kerollmops commented Jul 13, 2016 •

edited

Loading

jdroenner commented Jul 13, 2016

Kerollmops commented Jul 13, 2016

TeXitoi Jul 13, 2016

Kerollmops Jul 13, 2016

TeXitoi Jul 13, 2016

Kerollmops Jul 13, 2016

zarch commented Jul 14, 2016

Kerollmops commented Jul 14, 2016

frewsxcv commented Jul 16, 2016

frewsxcv Jul 16, 2016

frewsxcv commented Jul 16, 2016

frewsxcv Jul 16, 2016

frewsxcv commented Jul 17, 2016

frewsxcv commented Jul 18, 2016

Support generic numeric type for geometries #30

Support generic numeric type for geometries #30

Conversation

zarch commented May 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frewsxcv commented May 19, 2016

frewsxcv commented May 19, 2016

Choose a reason for hiding this comment

Kerollmops commented Jul 12, 2016

zarch commented Jul 13, 2016

Kerollmops commented Jul 13, 2016 • edited Loading

jdroenner commented Jul 13, 2016

Kerollmops commented Jul 13, 2016

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

zarch commented Jul 14, 2016

Kerollmops commented Jul 14, 2016

frewsxcv commented Jul 16, 2016

Choose a reason for hiding this comment

frewsxcv commented Jul 16, 2016

Choose a reason for hiding this comment

frewsxcv commented Jul 17, 2016

frewsxcv commented Jul 18, 2016

Kerollmops commented Jul 13, 2016 •

edited

Loading