Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

One Length trait to rule them all 💍 #256

Closed
frewsxcv opened this issue Jun 4, 2018 · 8 comments · Fixed by #1228
Closed

One Length trait to rule them all 💍 #256

frewsxcv opened this issue Jun 4, 2018 · 8 comments · Fixed by #1228
Labels

Comments

@frewsxcv
Copy link
Member

frewsxcv commented Jun 4, 2018

The following code is blocked on rust-lang/rust#51344. Relevant discussion happened in #47.

pub struct Line<T> {
    start: (T, T),
    end: (T, T),
}

////////////

pub trait LengthAlgo<T> {
    fn length(line: Line<T>) -> T;
}

////////////

pub enum Haversine {}

impl<T> LengthAlgo<T> for Haversine {
    fn length(line: Line<T>) -> T {
        unimplemented!()
    }
}

pub enum Vincenty {}

impl<T> LengthAlgo<T> for Vincenty {
    fn length(line: Line<T>) -> T {
        unimplemented!()
    }
}

////////////

pub trait Length<T, Algo: LengthAlgo<T> = Haversine> {
    fn length(self) -> T;
}

impl<T, Algo: LengthAlgo<T>> Length<T, Algo> for Line<T> {
    fn length(self) -> T {
        Algo::length(self)
    }
}

////////////

fn main() {
    let line: Line<i32> = Line {
        start: (4, 6),
        end: (2, 23),
    };
    
    // This doesn't compile – error message says `Algo` type needs to be specified
    // let haversine_length = line.length();
    
    // This compiles, despite us not specifying the type for `Algo`
    let haversine_length = Length::<i32>::length(line);
}
@frewsxcv frewsxcv added the idea label Jun 4, 2018
@frewsxcv
Copy link
Member Author

frewsxcv commented Jun 4, 2018

There are two topics that need to be discussed here:

  • What should the trait look like?
  • What should the default algorithm be?

@amandasaurus
Copy link
Member

amandasaurus commented Jun 4, 2018 via email

@frewsxcv
Copy link
Member Author

frewsxcv commented Jun 4, 2018

I have opinions about euclidean as a default, but short on time right now to expand.

Regarding taking ownership, you're right, it should take &self. Unfortunately can't make much progress here until my Rust issue is addressed :-/

@amandasaurus
Copy link
Member

amandasaurus commented Jun 5, 2018

PostGIS has a geography type (as opposed to it's older geometry) which stores everything in lat/lons and distance/area functions use a 'spherical/surface distance' and return values in metres. That's very useful in many cases.

Maybe a feature like that would be very useful for rust-geo, and avoid the "which should it be?" discussion?

@frewsxcv
Copy link
Member Author

frewsxcv commented Sep 9, 2023

As long as the underlying Rust issue remains open, I don't think we can make much progress on this, so I'm going to close.

@michaelkirk
Copy link
Member

With #1216 this starts to look pretty nice! See my WIP: 33436c5

Given all the unification in #1216, a unified Length trait is pretty concise to implement:

pub trait Length<F: CoordFloat> {
    fn length<MetricSpace: Distance<F, Point<F>, Point<F>>>(&self) -> F;
}
impl<F: CoordFloat> Length<F> for Line<F> {
    fn length<MetricSpace: Distance<F, Point<F>, Point<F>>>(&self) -> F {
        MetricSpace::distance(self.start_point(), self.end_point())
    }
}
impl<F: CoordFloat> Length<F> for LineString<F> {
    fn length<MetricSpace: Distance<F, Point<F>, Point<F>>>(&self) -> F {
        let mut length = F::zero();
        for line in self.lines() {
            length = length + line.length::<MetricSpace>();
        }
        length
    }
}

And then the call sites look like this:

let line_string = LineString::new(vec![
    coord!(x: -58.3816f64, y: -34.6037), // Buenos Aires, Argentina
    coord!(x: -77.0428, y: -12.0464),    // Lima, Peru
    coord!(x: -47.9292, y: -15.7801),    // Brasília, Brazil
]);
assert_eq!(
    6_302_220., // meters
    line_string.length::<Geodesic>().round()
);
assert_eq!(
    6_332_790., // meters
    line_string.length::<Rhumb>().round()
);
assert_eq!(
    6_304_387., // meters
    line_string.length::<Haversine>().round()
);
// computing Euclidean length of an unprojected (lng/lat) gives a nonsense answer
assert_eq!(
    59., // nonsense!
    line_string.length::<Euclidean>().round()
);
// EPSG:102033
let projected_line_string = LineString::from(vec![
    coord!(x: 143042.46f64, y: -1932485.45), // Buenos Aires, Argentina
    coord!(x: -1797084.08, y: 583528.84),    // Lima, Peru
    coord!(x: 1240052.27, y: 207169.12),     // Brasília, Brazil
]);
assert_eq!(
    6_237_538.,
    projected_line_string.length::<Euclidean>().round()
);

@michaelkirk
Copy link
Member

michaelkirk commented Sep 26, 2024

I'll call out that Vincenty is awkward because it returns a Result<T, DoesNotConvergeError>, so I haven't included it in my first pass at "one length trait to rule them all".

Maybe we can do something fancy, but I haven't thought very hard about it yet.

@michaelkirk
Copy link
Member

I'm also diverging from your original approach by not including a default trait implementation.

I feel like people using geographic (lon/lat) coords and people using projected coordinates are both big enough groups that picking a default is too much of a footgun.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants