Use Angle trait for lat/lng Points #95

Turbo87 · 2017-02-19T10:56:03Z

I've built an initial spike to use the Angle trait from the cgmath crate to represent lat/lng points.

Solved: I'm flagging this as work-in-progress as I can't seem to be able to make impl<T> Neg for Point<T> where T: Angle work. It just fails to compile telling me conflicting implementations of trait `std::ops::Neg` for type `types::Point<_>` . It's been a long time since I last used Rust and if anyone has ideas on how to solve this I'd be happy to hear them.

The structs may have different types, but we can still limit the implementations to certain types

Turbo87 · 2017-02-20T20:39:11Z

@frewsxcv it looks like I've actually managed to implement Neg, Add and Sub generically now and I think this PR is now ready for review

… Add

frewsxcv · 2017-02-20T21:32:56Z

Would it be cleaner if cgmath::Deg implemented Float + ToPrimitive? Maybe I/we should open a PR to cgmath

Turbo87 · 2017-02-20T23:03:03Z

Would it be cleaner if cgmath::Deg implemented Float + ToPrimitive?

I'm not sure if that would be useful tbh

frewsxcv · 2017-02-21T13:54:05Z

Would it be cleaner if cgmath::Deg implemented Float + ToPrimitive?

I'm not sure if that would be useful tbh

If Deg implemented Float + ToPrimitive, wouldn't it be able to use rust-geo without any changes?

I'm asking because it seems unfortunate that we have to duplicate some of these definitions. Makes it more confusing to have two ways to do something when we already have a very generic way to use rust-geo with Float.

Turbo87 · 2017-02-21T14:02:29Z

@frewsxcv I'm actually not duplicating any implementations anymore, I've just adjusted what implementations are valid for what kinds of traits

The problem with having Deg implement Float + ToPrimitive is that you would not gain any advantages from that. The main advantage here is being able to distinguish between radians and degrees, but if everything is handled as floats underneath without caring for their special meaning then we don't gain any advantages.

Turbo87 · 2017-02-28T09:27:32Z

@frewsxcv any more thoughts on this? should we merge it?

frewsxcv · 2017-02-28T14:03:58Z

@Turbo87 Sorry for taking so long on this, I've been pretty busy recently. I've already looked through the code for this and it seems fine, I just want to git checkout your branch and play around with it. I'll try to review it sometime today, thanks for being patient :)

Turbo87 · 2017-02-28T14:17:46Z

cool, thanks!

after merging this we should try to finish up #85, otherwise the conflicts won't get any smaller 😉

frewsxcv · 2017-03-01T02:31:37Z

src/types.rs


+impl<T> Point<T>
+    where T: Copy


Not all numeric types implement Copy, for example BigInt.

frewsxcv · 2017-03-01T02:45:41Z

src/types.rs

    ///
-    /// assert_eq!(p.lng(), 1.234);
+    /// assert_eq!(p.lng(), Deg(1.234));
+    /// # }
    /// ```
    pub fn lng(&self) -> T {


Why is lng only implemented for implementors of Angle? What about for other types that implement Float?

frewsxcv · 2017-03-01T02:49:06Z

src/types.rs

+    /// assert_eq!(p.lat(), Deg(0.5));
+    /// # }
+    /// ```
+    /// ```


Extra line of backticks here

frewsxcv · 2017-03-01T03:05:19Z

Cargo.toml

@@ -11,6 +11,7 @@ keywords = ["gis", "geo", "geography", "geospatial"]

 [dependencies]
 num-traits = "0.1"
+cgmath = "^0.12.0"


FYI ^ is redundant here. In a Cargo.toml file, 0.12.0 and ^0.12.0 are the same thing

http://doc.crates.io/specifying-dependencies.html#specifying-dependencies-from-cratesio

didn't know that, thanks!

frewsxcv · 2017-03-01T03:21:24Z

Just to take a step back here, what's the need for explicitly supporting this specific trait?

Is mixing up radians and degrees a common problem when doing geospatial operations? (This is an actual question, I really don't know the answer)

If so, it seems easy to get around this by one having their own Degree and Radian type that both implement Float. Right now, one cannot do Point<X> + Point<Y>, they have to be the same type.

Turbo87 · 2017-03-01T08:53:36Z

okay, so let me start from the beginning. in most serialization formats degrees are used and if angles are presented to the user on the screen degrees are used too. in calculations however (sin, cos, etc.) the computer usually expects radians (see e.g. https://github.com/georust/rust-geo/blob/master/src/algorithm/haversine_distance.rs#L30-L31). if you only do a few calculations then converting from deg to rad for each calculation is fine, but if you do lots of calculations that will add up to a significant overhead.

that is basically the reason why it can make sense for some users to read the angles as degrees, but immediately convert to radians because they will be doing lots of calculations with them and only convert to degree again if the angles are serialized or shown to the user at the end.

Right now, one cannot do Point + Point, they have to be the same type.

if Degree and Radian would implement Float and Float implemented Add then Degree(90) + Radian(0.5) would result in Float(90.5) which is obviously wrong. The same would be the case for Point<Degree> and Point<Radian>.

Please let me know if that answers your questions.

frewsxcv · 2017-03-01T14:17:31Z

Regarding your first couple paragraphs, I think you make valid points, but I need to digest and think about it a little bit.

if Degree and Radian would implement Float and Float implemented Add then Degree(90) + Radian(0.5) would result in Float(90.5) which is obviously wrong.

If this were the case, I'd be more in favor of this pull request. Looking at the docs, it looks like the Float inherits from the Num trait. It also looks like Num inherits from Add<Output=Self> and since RHS isn't specified here, the type being added is the same as self, meaning that Add only works in situations where the left-hand-side and the right-hand-side are the same type:

extern crate num_traits;                                                                               
                                                     
// compiles                                                  
fn add_floats_1<X: num_traits::Float>(x: X, y: X) {                                                    
    let _ = x + y;                                                                                     
}                                                                                                      
         
// does not compile                                                                         
fn add_floats<X: num_traits::Float, Y: num_traits::Float>(x: X, y: Y) {                                
    let _ = x + y;                                                                                     
}                                                                                                      
                                                                                                       
fn main() {}

Turbo87 · 2017-06-13T22:18:29Z

@frewsxcv any news on this?

frewsxcv · 2017-06-20T04:26:41Z

Sorry for the delay here. This is on my radar, I just haven't had time to look at this but I still plan to :-/

Turbo87 added 5 commits February 19, 2017 09:16

Add "cgmath" dependency

8f87a55

types: Remove "where T: float" from struct declarations

b46b926

The structs may have different types, but we can still limit the implementations to certain types

types: Split x/y from lat/lng Point implementations

13cf6c6

haversine_distance: Change implementation to use Angle trait

7676b20

types: Implement Neg, Add and Sub traits for Points generically

a9fa0ee

Turbo87 changed the title ~~WIP: Use Angle trait for lat/lng Points~~ Use Angle trait for lat/lng Points Feb 20, 2017

haversine_distance: Implement HaversineDistance trait similar to e.g.…

832dcf6

… Add

Turbo87 added 3 commits February 21, 2017 08:56

types: Implement x/y() and set_x/y() for Copy trait

a4a514b

types: Move dot() implementation further down

f211149

types: Reuse x/y() getters and setters

28ca717

frewsxcv reviewed Mar 1, 2017

View reviewed changes

src/types.rs

impl<T> Point<T>

where T: Copy

Copy link

Member

frewsxcv Mar 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all numeric types implement Copy, for example BigInt.

frewsxcv reviewed Mar 1, 2017

View reviewed changes

Turbo87 closed this Aug 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Angle trait for lat/lng Points #95

Use Angle trait for lat/lng Points #95

Turbo87 commented Feb 19, 2017 •

edited

Loading

Turbo87 commented Feb 20, 2017

frewsxcv commented Feb 20, 2017

Turbo87 commented Feb 20, 2017

frewsxcv commented Feb 21, 2017

Turbo87 commented Feb 21, 2017

Turbo87 commented Feb 28, 2017

frewsxcv commented Feb 28, 2017

Turbo87 commented Feb 28, 2017

frewsxcv Mar 1, 2017

frewsxcv Mar 1, 2017

frewsxcv Mar 1, 2017

frewsxcv Mar 1, 2017

Turbo87 Mar 1, 2017

frewsxcv commented Mar 1, 2017

Turbo87 commented Mar 1, 2017

frewsxcv commented Mar 1, 2017

Turbo87 commented Jun 13, 2017

frewsxcv commented Jun 20, 2017

Use Angle trait for lat/lng Points #95

Use Angle trait for lat/lng Points #95

Conversation

Turbo87 commented Feb 19, 2017 • edited Loading

Turbo87 commented Feb 20, 2017

frewsxcv commented Feb 20, 2017

Turbo87 commented Feb 20, 2017

frewsxcv commented Feb 21, 2017

Turbo87 commented Feb 21, 2017

Turbo87 commented Feb 28, 2017

frewsxcv commented Feb 28, 2017

Turbo87 commented Feb 28, 2017

frewsxcv Mar 1, 2017

Choose a reason for hiding this comment

frewsxcv Mar 1, 2017

Choose a reason for hiding this comment

frewsxcv Mar 1, 2017

Choose a reason for hiding this comment

frewsxcv Mar 1, 2017

Choose a reason for hiding this comment

Turbo87 Mar 1, 2017

Choose a reason for hiding this comment

frewsxcv commented Mar 1, 2017

Turbo87 commented Mar 1, 2017

frewsxcv commented Mar 1, 2017

Turbo87 commented Jun 13, 2017

frewsxcv commented Jun 20, 2017

Turbo87 commented Feb 19, 2017 •

edited

Loading