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 measurement traits #1181

Open
michaelkirk opened this issue May 17, 2024 · 7 comments · May be fixed by #1216
Open

Unify line measurement traits #1181

michaelkirk opened this issue May 17, 2024 · 7 comments · May be fixed by #1216

Comments

@michaelkirk
Copy link
Member

michaelkirk commented May 17, 2024

We have a lot of algorithms (bearing, destination, distance, etc.) implemented across different metric spaces (haversine, euclidean, geodesic), and the current arrangement, arrived at organically, can be pretty confusing.

e.g. It took me a while to realize that we already have haversine corollaries to some euclidean methods. Same thing in #1208

I think we can arrange things differently to make this more obvious and discoverable to users

EDIT: I've deleted my original proposal, and have a new one.

New Proposal

Introduce a struct per metric space:

struct Haversine;
struct Rhumb;
struct Geodesic;
struct Euclidean;

Introduce a unified trait per algorithm, and implement that trait on each struct:

trait Bearing<T: CoordFloat> {
  fn bearing(p1: Point<T>, p2: Point<T>) -> T
}

impl Bearing for Haversine {
  fn bearing(p1: Point<T>, p2: Point<T>) -> T {
    todo!()
  }
}
impl Bearing for Rhumb {
  fn bearing(p1: Point<T>, p2: Point<T>) -> T {
    todo!()
  }
}
impl Bearing for Geodesic {
  fn bearing(p1: Point<T>, p2: Point<T>) -> T {
    todo!()
  }
}
impl Bearing for Euclidean {
  fn bearing(p1: Point<T>, p2: Point<T>) -> T {
    todo!()
  }
}

Notes

Current Measurement Traits

Here our existing traits that seem at least somewhat related to the notion at hand. I don't intend to unify all of these, but there are some definite patterns that I think we can capture to make things more intuitive to people.

As a csv: measurements.csv

Rendered:

Trait area_signed area_unsigned bearing closest_point densify destination distance intermediate intermediate_fill length (➡Oddballs ➡) interpolate_point cross_track_distance bearing_distance perimeter perimeter_and_area_signed perimeter_and_area_unsigned
GeodesicArea geodesic_area_signed geodesic_area_unsigned geodesic_perimeter geodesic_perimeter_area_signed geodesic_perimeter_area_unsigned
GeodesicBearing geodesic_bearing geodesic_bearing_distance
GeodesicDestination geodesic_destination
GeodesicDistance geodesic_distance (point to point only)
GeodesicIntermediate geodesic_intermediate geodesic_intermediate_fill
GeodesicLength geodesic_length
HaversineBearing haversine_bearing
HaversineClosestPoint haversine_closest_point
DensifyHaversine densify_haversine
HaversineDestination haversine_destination
HaversineDistance haversine_distance (point to point only)
HaversineIntermediate haversine_intermediate haversine_intermediate_fill
HaversineLength haversine_length
Area (Euclidean) signed_area unsigned_area
ClosestPoint (Euclidean) closest_point
Densify (Euclidean) densify (euclidean)
EuclideanDistance euclidean_distance (all geoms)
EuclideanLength euclidean_length
RhumbBearing rhumb_bearing
RhumbDestination rhumb_destination
RhumbDistance rhumb_distance
RhumbIntermediate rhumb_intermediate rhumb_intermediate_fill
RhumbLength rhumb_length
(⬇Oddballs ⬇)
HausdorffDistance hausdorff_distance (point to point only)
VincentyDistance vincenty_distance
VincentyLength vincenty_length
CrossTrackDistance cross_track_distance (haversine)
LineInterpolatePoint line_interpolate_point (euclidean)

/cc @JivanRoquet original author of the HaversineIntermediate trait in #230
/cc @JosiahParry original author of the DensifyHaversine trait in #1081

@michaelkirk
Copy link
Member Author

michaelkirk commented May 17, 2024

Oh man, the plot thickens.

Maybe we shouldn't just get rid of HaversineIntermediate, because there are sibling traits: GeodesicIntermediate and RhumbIntermediate.

I don't have a solution yet and have run out of time to think about this today, but I'm open to proposals about how to re-align things to increase uniformity and decrease redundancy.

@JosiahParry
Copy link
Contributor

One option would be to have an IntermediatePoint trait that requires an enum like DistanceType::Rhumb, DistanceType::Haversine, DistanceType::Euclidean, DistanceType::Geodesic. Then, for simplicity have convenience functions in the module.

fn intermediate_haversine(start: &Point, end: &Point, prop: f64) {}
fn intermediate_geodesic(start: &Point, end: &Point, prop: f64) {}
fn intermediate_euclidean(start: &Point, end: &Point, prop: f64) {}
fn intermediate_rhumb(start: &Point, end: &Point, prop: f64) {}

@michaelkirk
Copy link
Member Author

I spent a while trying to understand our existing "measurement" traits - see the new table in the issue description.

It exposes a bit of overlap, some inconsistencies, and some missing functionality. There are also some "similar/related" traits that don't exactly fit the pattern, which I've labeled as "oddballs".

@michaelkirk
Copy link
Member Author

michaelkirk commented Sep 21, 2024

Next up, I want to collapse these into some kind of conceptual "metric space" trait:

Screenshot 2024-09-20 at 17 56 44

(WIP)

I don't know that it will actually be implementable as such, but if nothing else it will be helpful documentation. I am hopeful that it will lead to some clarify about how to more coherently organize our implementations as well.

@michaelkirk michaelkirk changed the title Replace HaversineIntermediate with HaversineLineInterpolatePoint and HaversineDensify Unify line measurement traits Sep 24, 2024
@michaelkirk
Copy link
Member Author

I've rescoped this issue a bit and fleshed out some of the details. I hope to have the first PR improving (I think!) the situation by the end of today.

@frewsxcv
Copy link
Member

frewsxcv commented Sep 25, 2024

Next up, I want to collapse these into some kind of conceptual "metric space" trait:

Screenshot 2024-09-20 at 17 56 44 (WIP)

I don't know that it will actually be implementable as such, but if nothing else it will be helpful documentation. I am hopeful that it will lead to some clarify about how to more coherently organize our implementations as well.

Don't forget EuclideanLength et al. See also #256.

michaelkirk added a commit that referenced this issue Sep 25, 2024
All of these algorithm implementations already existed, but I'm
proposing a new way to organize them which I hope will be more
consistent and discoverable.

Note: The new Haversine::bearing and Geodesic::bearing return 0..360,
the legacy traits HaversineBearing and GeodesicBearing returned
-180..180

Additional changes:

Deleted the deprecated `Bearing` trait which was previously superceeded
by the unambiguous `HaversineBearing` trait, but now is re-defined as
`Haversine::bearing`

= Future Work =

In an effort to minimize this PR, while keeping the change reasonably
coherent, I've left some things out

== Methods on Euclidean ==
 -[ ] bearing (doesn't currently exist)
 -[ ] destination (doesn't currently exist)
 -[ ] intermediate (exists, but InterpolatePoint trait also needs intermediate_fill)
 -[ ] intermediate_fill (doesn't currently exist)

== Deprecate Legacy Traits ==
 -[ ] Deprecate Legacy impls
 -[ ] Switcheroo the actual implementation: move the actual implementation to the new traits, and have the legacy traits delegate to the new traits.
 -[ ] Move over any tests from the legacy implementation to the new home

== Methods on Geoms (Future PR) ==
 -[ ] Length
     -[ ] Haversine
     -[ ] Rhumb
     -[ ] Geodesic
     -[ ] Euclidean
 -[ ] Densify
     -[ ] Haversine
     -[ ] Rhumb
     -[ ] Geodesic
     -[ ] Euclidean

FIXES #1210
FIXES #1181
michaelkirk added a commit that referenced this issue Sep 25, 2024
All of these algorithm implementations already existed, but I'm
proposing a new way to organize them which I hope will be more
consistent and discoverable.

Note: The new Haversine::bearing and Geodesic::bearing return 0..360,
the legacy traits HaversineBearing and GeodesicBearing returned
-180..180

Additional changes:

Deleted the deprecated `Bearing` trait which was previously superceeded
by the unambiguous `HaversineBearing` trait, but now is re-defined as
`Haversine::bearing`

= Future Work =

In an effort to minimize this PR, while keeping the change reasonably
coherent, I've left some things out

== Methods on Euclidean ==
 -[ ] bearing (doesn't currently exist)
 -[ ] destination (doesn't currently exist)
 -[ ] intermediate (exists, but InterpolatePoint trait also needs intermediate_fill)
 -[ ] intermediate_fill (doesn't currently exist)

== Deprecate Legacy Traits ==
 -[ ] Deprecate Legacy impls
 -[ ] Switcheroo the actual implementation: move the actual implementation to the new traits, and have the legacy traits delegate to the new traits.
 -[ ] Move over any tests from the legacy implementation to the new home

== Methods on Geoms (Future PR) ==
 -[ ] Length
     -[ ] Haversine
     -[ ] Rhumb
     -[ ] Geodesic
     -[ ] Euclidean
 -[ ] Densify
     -[ ] Haversine
     -[ ] Rhumb
     -[ ] Geodesic
     -[ ] Euclidean

FIXES #1210
FIXES #1181
michaelkirk added a commit that referenced this issue Sep 25, 2024
All of these algorithm implementations already existed, but I'm
proposing a new way to organize them which I hope will be more
consistent and discoverable.

Note: The new Haversine::bearing and Geodesic::bearing return 0..360,
the legacy traits HaversineBearing and GeodesicBearing returned
-180..180

Additional changes:

Deleted the deprecated `Bearing` trait which was previously superceeded
by the unambiguous `HaversineBearing` trait, but now is re-defined as
`Haversine::bearing`

= Future Work =

In an effort to minimize this PR, while keeping the change reasonably
coherent, I've left some things out

== Methods on Euclidean ==
 -[ ] bearing (doesn't currently exist)
 -[ ] destination (doesn't currently exist)
 -[ ] intermediate (exists, but InterpolatePoint trait also needs intermediate_fill)
 -[ ] intermediate_fill (doesn't currently exist)

== Deprecate Legacy Traits ==
 -[ ] Deprecate Legacy impls
 -[ ] Switcheroo the actual implementation: move the actual implementation to the new traits, and have the legacy traits delegate to the new traits.
 -[ ] Move over any tests from the legacy implementation to the new home

== Methods on Geoms (Future PR) ==
 -[ ] Length
     -[ ] Haversine
     -[ ] Rhumb
     -[ ] Geodesic
     -[ ] Euclidean
 -[ ] Densify
     -[ ] Haversine
     -[ ] Rhumb
     -[ ] Geodesic
     -[ ] Euclidean

FIXES #1210
FIXES #1181
@michaelkirk michaelkirk linked a pull request Sep 25, 2024 that will close this issue
2 tasks
@michaelkirk
Copy link
Member Author

Don't forget EuclideanLength et al.

It's in there! We don't currently have implementations for all the algorithms for Euclidean, but I'd like to add them in a followup.

See also #256.

Ah yes! I forgot about that issue. I just opened #1216 which takes a similar direction, though I haven't included the length trait yet. It's already a huge PR, so I'm trying to omit things when possible, while still keeping the codebase coherent enough to be deployed.

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

Successfully merging a pull request may close this issue.

3 participants