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

Unify line measures #1216

Merged
merged 11 commits into from
Oct 8, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ jobs:
# giving us about 6 months of coverage.
#
# Minimum supported rust version (MSRV)
- "ghcr.io/georust/geo-ci:proj-9.4.0-rust-1.74"
- "ghcr.io/georust/geo-ci:proj-9.4.0-rust-1.75"
# Two most recent releases - we omit older ones for expedient CI
- "ghcr.io/georust/geo-ci:proj-9.4.0-rust-1.78"
- "ghcr.io/georust/geo-ci:proj-9.4.0-rust-1.79"
Expand Down Expand Up @@ -105,7 +105,7 @@ jobs:
# giving us about 6 months of coverage.
#
# Minimum supported rust version (MSRV)
- "ghcr.io/georust/geo-ci:proj-9.4.0-rust-1.74"
- "ghcr.io/georust/geo-ci:proj-9.4.0-rust-1.75"
# Two most recent releases - we omit older ones for expedient CI
- "ghcr.io/georust/geo-ci:proj-9.4.0-rust-1.78"
- "ghcr.io/georust/geo-ci:proj-9.4.0-rust-1.79"
Expand All @@ -132,7 +132,7 @@ jobs:
# giving us about 6 months of coverage.
#
# Minimum supported rust version (MSRV)
- "ghcr.io/georust/geo-ci:proj-9.4.0-rust-1.74"
- "ghcr.io/georust/geo-ci:proj-9.4.0-rust-1.75"
# Two most recent releases - we omit older ones for expedient CI
- "ghcr.io/georust/geo-ci:proj-9.4.0-rust-1.78"
- "ghcr.io/georust/geo-ci:proj-9.4.0-rust-1.79"
Expand Down
2 changes: 1 addition & 1 deletion geo-postgis/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ documentation = "https://docs.rs/geo-postgis/"
readme = "../README.md"
keywords = ["gis", "geo", "geography", "geospatial", "postgis"]
description = "Conversion between `geo-types` and `postgis` types."
rust-version = "1.74"
rust-version = "1.75"
edition = "2021"

[dependencies]
Expand Down
2 changes: 1 addition & 1 deletion geo-types/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ documentation = "https://docs.rs/geo-types/"
readme = "../README.md"
keywords = ["gis", "geo", "geography", "geospatial"]
description = "Geospatial primitive data types"
rust-version = "1.74"
rust-version = "1.75"
edition = "2021"

[features]
Expand Down
20 changes: 20 additions & 0 deletions geo/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,26 @@
* <https://github.com/georust/geo/pull/1201>
* Add `StitchTriangles` trait which implements a new kind of combining algorithm for `Triangle`s
* <https://github.com/georust/geo/pull/1087>
* BREAKING: Remove deprecated `Bearing` trait
* Unify various line measurements under new `line_measures::{Bearing, Distance, Destination, InterpolatePoint}` traits
Before:
```
use geo::{GeodesicBearing, HaversineBearing, GeodesicDistance, HaversineDistance};
GeodesicBearing::geodesic_bearing(p1, p2)
HaversineBearing::haversine_bearing(p1, p2)
GeodesicDistance::geodesic_distance(p1, p2)
HaversineDistance::haversine_distance(p1, p2)
```

After:
```
use geo::{Geodesic, Haversine, Bearing, Distance};
Geodesic::bearing(p1, p2)
Haversine::bearing(p1, p2)
Geodesic::distance(p1, p2)
Haversine::distance(p1, p2)
```
* <https://github.com/georust/geo/pull/TBD>

## 0.28.0

Expand Down
2 changes: 1 addition & 1 deletion geo/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ readme = "../README.md"
keywords = ["gis", "geo", "geography", "geospatial"]
autobenches = true
edition = "2021"
rust-version = "1.74"
rust-version = "1.75"
categories = ["science::geo"]

[features]
Expand Down
33 changes: 0 additions & 33 deletions geo/src/algorithm/bearing.rs

This file was deleted.

2 changes: 0 additions & 2 deletions geo/src/algorithm/geodesic_intermediate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use crate::{CoordFloat, Point};
use geographiclib_rs::{DirectGeodesic, Geodesic, InverseGeodesic};

/// Returns a new Point along a route between two existing points on an ellipsoidal model of the earth

pub trait GeodesicIntermediate<T: CoordFloat> {
/// Returns a new Point along a route between two existing points on an ellipsoidal model of the earth
///
Expand All @@ -25,7 +24,6 @@ pub trait GeodesicIntermediate<T: CoordFloat> {
/// assert_relative_eq!(i50, i50_should, epsilon = 1.0e-6);
/// assert_relative_eq!(i80, i80_should, epsilon = 1.0e-6);
/// ```

fn geodesic_intermediate(&self, other: &Point<T>, f: T) -> Point<T>;
fn geodesic_intermediate_fill(
&self,
Expand Down
13 changes: 13 additions & 0 deletions geo/src/algorithm/line_measures/bearing.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
use geo_types::{CoordFloat, Point};

/// Calculate the bearing between two points
pub trait Bearing<F: CoordFloat> {
/// Calculate the bearing from `origin` to `destination` in degrees.
///
/// See [specific implementations](#implementors) for details.
///
/// # Units
/// - `origin`, `destination`: Point where the units of x/y depend on the [trait implementation](#implementors).
/// - returns: degrees, where: North: 0°, East: 90°, South: 180°, West: 270°
fn bearing(origin: Point<F>, destination: Point<F>) -> F;
}
19 changes: 19 additions & 0 deletions geo/src/algorithm/line_measures/destination.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
use geo_types::{CoordFloat, Point};

/// Calculate the destination point from an origin point, a bearing and a distance.
pub trait Destination<F: CoordFloat> {
/// Returns a new point having travelled the `distance` along a line
/// from the `origin` point with the given `bearing`.
///
/// See [specific implementations](#implementors) for details.
///
/// # Units
///
/// - `origin`: Point where the units of x/y depend on the [trait implementation](#implementors).
/// - `bearing`: degrees, where: North: 0°, East: 90°, South: 180°, West: 270°
/// - `distance`: depends on the [trait implementation](#implementors).
/// - returns: Point where the units of x/y depend on the [trait implementation](#implementors).
///
/// [`metric_spaces`]: super::metric_spaces
fn destination(origin: Point<F>, bearing: F, distance: F) -> Point<F>;
}
12 changes: 12 additions & 0 deletions geo/src/algorithm/line_measures/distance.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/// Calculate the distance between the `Origin` and `Destination` geometry.
pub trait Distance<F, Origin, Destination> {
/// Note that not all implementations support all geometry combinations, but at least `Point` to `Point`
/// is supported.
/// See [specific implementations](#implementors) for details.
///
/// # Units
///
/// - `origin`, `destination`: geometry where the units of x/y depend on the trait implementation.
/// - returns: depends on the trait implementation.
fn distance(origin: Origin, destination: Destination) -> F;
}
34 changes: 34 additions & 0 deletions geo/src/algorithm/line_measures/interpolate_point.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
use crate::{CoordFloat, Point};

// REVIEW: Naming alternatives:
// - LinearReferencing
// - PointAlongLine
// - LineInterpolatePoint (postgis)
// - Interpolate (shapely)
// - Position (geographiclib)
// - Intermediate (georust::geo)
pub trait InterpolatePoint<F: CoordFloat> {
/// Returns a new Point along a line between two existing points
///
/// See [specific implementations](#implementors) for details.
fn point_at_ratio_between(start: Point<F>, end: Point<F>, ratio_from_start: F) -> Point<F>;

// TODO:
// fn point_at_distance_between(start: Point<F>, end: Point<F>, distance_from_start: F) -> Point<F>;

/// Interpolates `Point`s along a line between `start` and `end`.
///
/// See [specific implementations](#implementors) for details.
///
/// As many points as necessary will be added such that the distance between points
/// never exceeds `max_distance`. If the distance between start and end is less than
/// `max_distance`, no additional points will be included in the output.
///
/// `include_ends`: Should the start and end points be included in the output?
fn points_along_line(
start: Point<F>,
end: Point<F>,
max_distance: F,
include_ends: bool,
) -> impl Iterator<Item = Point<F>>;
}
71 changes: 71 additions & 0 deletions geo/src/algorithm/line_measures/metric_spaces/euclidean.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
use super::super::Distance;
use crate::{GeoFloat, Point};

/// Euclidean space measures distance with the pythagorean formula - what you'd measure with a ruler.
///
/// You must use projected coordinates with Euclidean space —
Copy link
Member

Choose a reason for hiding this comment

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

I know some people have complained about excessively technical language (I disagree), but I'm uneasy about using terminology like "ruler" or "projected coordinates". I think we should refer to this space and link to it as The Euclidean Plane, specifically "the set R2 of the ordered pairs of real numbers, equipped with the dot product", because it's unambiguous. We could also provide examples of what it isn't (I'm not particularly in favour, but I can see why it might be helpful).

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that euclidean plane link - I'll work that in.

I think that your very technical description will be, on average, more confusing for someone (like me) who just wants "regular old normal measurements like I learned to do in middle school geometry".

My impression is that you are trying to target a more mathematically adroit user with your precise definition, which is a reasonable segment to target, but at the cost of confusing users like me... I'm not sure it's worth it.

Is there plausible confusion here? What other metric space would people think we're talking about?

Copy link
Member Author

Choose a reason for hiding this comment

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

Phrased a little differently:

I think the most likely mistake people will make is trying to put GPS coordinates into a euclidean method or vice-versa.

So rather than giving a complete dictionary definition of what a thing is I'm optimizing to have documentation that I think will lead to the fewest number of errors based on peoples short attention spans.

Copy link
Member

Choose a reason for hiding this comment

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

I think the most likely mistake people will make is trying to put GPS coordinates into a euclidean method or vice-versa.

I agree, but I also want to avoid using language that's going to be confusing to people who aren't familiar with coordinate systems. Fundamentally, we want people to know what the rules of the system they using are (because we are declining to enforce the rules using the type system)

I want people to know they're in The Euclidean Plane, because that's unambiguous information that they can expand upon if they're unsure, and it defines the rules that are in use w/r/t/ distance etc.

For people who are implementing algorithms using geo, they will want to know they're in R2 because the literature that you have to read to implement them often states this as an assumption.

So I would say I don't think we disagree about who we're targeting (the widest possible range of users).

/// for lon/lat points, use the [`Haversine`], [`Geodesic`], or other [metric spaces].
///
/// [`Haversine`]: super::Haversine
/// [`Geodesic`]: super::Geodesic
/// [metric spaces]: super
pub struct Euclidean;

/// Calculate the Euclidean distance (a.k.a. pythagorean distance) between two Points
impl<F: GeoFloat> Distance<F, Point<F>, Point<F>> for Euclidean {
/// Calculate the Euclidean distance (a.k.a. pythagorean distance) between two Points
///
/// # Units
/// - `origin`, `destination`: Point where the units of x/y represent non-angular units
/// — e.g. meters or miles, not lon/lat. For lon/lat points, use the
Copy link
Member

Choose a reason for hiding this comment

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

In a similar vein, I don't think we should be referring to "meters or miles" here: non-Euclidean distances are often measured in meters / miles, so this is confusing. "Not lon/lat" is OK, but I'd prefer that this is either implicit (you can't measure lon / lat distances in Euclidean space, and we are in Euclidean space, therefore…) or that we decline to explain with physical-world examples completely.

Copy link
Member Author

Choose a reason for hiding this comment

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

non-Euclidean distances are often measured in meters / miles, so this is confusing.

I believe you, but I honestly didn't know that was true. Can you give me an example?

you can't measure lon / lat distances in Euclidean space

Oh but you can! And it's a common error that gives you a meaningless result. Look no further than our own example code on the legacy euclidean distance:

let p1 = point!(x: -72.1235, y: 42.3521);
let p2 = point!(x: -72.1260, y: 42.45);
let distance = p1.euclidean_distance(&p2);

assert_relative_eq!(distance, 0.09793191512474639);

I think it's important to keep simple relatable language and examples to help mitigate that.

Copy link
Member

Choose a reason for hiding this comment

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

I believe you, but I honestly didn't know that was true. Can you give me an example?

Any physical measurement of distance on the earth's surface (or a simplified abstraction of it) is definitionally non-Euclidean because it's a curved surface.

Copy link
Member Author

@michaelkirk michaelkirk Sep 25, 2024

Choose a reason for hiding this comment

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

Ah I see, that makes sense. Thank you for bearing with me. I don't have a formal GIS background, so a lot of things that may seem obvious, I truly don't know.

I've included the link to [Euclidean Plane], which I hope addresses your ambiguity concern.

I've also linked to the Transform trait in reference to what we mean by "projection". While also not 100% precise, I think that it will be helpful to those who know a little, without confusing those who know a lot.

I still think it's important that our documentation aim to help people avoid the most likely errors by saying something like:

this method takes lng/lat

But the negation:

this method does not take lng/lat

Feels awkward and unnecessarily mysterious without an example. I now understand how Euclidean space is not the only space that could use meters as a unit, but as an example of what we mean by "not lng/lat", I think it is clarifying, and unlikely to confuse.

So if I've said something that is incorrect, let me know and I'll try again to fix it. But if it's more so that you think it reads like it was written for dummies, well then I'd please beg of you to have mercy on us dummies.

/// [`Haversine`] or [`Geodesic`] [metric spaces].
/// - returns: distance in the same units as the `origin` and `destination` points
///
/// # Example
/// ```
/// use geo::{Euclidean, Distance};
/// use geo::Point;
/// // web mercator
/// let new_york_city = Point::new(-8238310.24, 4942194.78);
/// // web mercator
/// let london = Point::new(-14226.63, 6678077.70);
/// let distance: f64 = Euclidean::distance(new_york_city, london);
///
/// assert_eq!(
/// 8_405_286., // meters in web mercator
/// distance.round()
/// );
/// ```
///
/// [`Haversine`]: super::Haversine
/// [`Geodesic`]: super::Geodesic
/// [metric spaces]: super
fn distance(origin: Point<F>, destination: Point<F>) -> F {
crate::EuclideanDistance::euclidean_distance(&origin, &destination)
}
}

#[cfg(test)]
mod tests {
use super::*;

type MetricSpace = Euclidean;

mod distance {
use super::*;

#[test]
fn new_york_to_london() {
// web mercator
let new_york_city = Point::new(-8238310.24, 4942194.78);
// web mercator
let london = Point::new(-14226.63, 6678077.70);
let distance: f64 = MetricSpace::distance(new_york_city, london);

assert_relative_eq!(
8_405_286., // meters in web mercator
distance.round()
);
}
}
}
Loading
Loading