georust / geo

Geospatial primitives and algorithms for Rust
https://crates.io/crates/geo
Other
1.47k stars 190 forks source link

Replace HaversineIntermediate with HaversineLineInterpolatePoint and HaversineDensify #1181

Open michaelkirk opened 1 month ago

michaelkirk commented 1 month ago

We have a lot of traits! It took me a while to realize that we had haversine corollaries to these euclidean methods. I think we can arrange things differently to make this more obvious and discoverable to users (like me), like we do with {Euclidean|Haversine}Length and {Euclidean|Haversine}Distance traits.

HaversineIntermediate.haversine_intermediate_fill is the same thing as the existing DensifyHaversine, right?

HaversineIntermediate.haversine_intermediate is pretty much the haversine corollary to the Euclidean LineInterpolatePoint

Proposal:

Let's delete HaversineIntermediate (after a deprecation cycle). HaversineIntermediate .haversine_intermediate will live on in a new trait HaversineLineInterpolatePoint that more closely mirrors (Euclidiean)LineInterpolatePoint and HaversineIntermediate .haversine_intermediate_fill will go away because it's redundant with DensifyHaversine.

This won't be completely trivial because the API's don't exactly align, but it's not much of a stretch.

One difference: HaversineIntermediate.haversine_intermediate_fill takes two points as input, so conceptually it only works on a Line, whereas DensifyHaversine is implemented on other geometry types, like Polygon, but I think that's more of a benefit than anything.

Another difference: HaversineIntermediate.haversine_intermediate_fill has an includes_end parameters. We could add a similar parameter or method flavor to the existing Densify trait to match this new proposed HaversineDensify, but my instinct is actually just to get rid of the parameter and pretend like it's always true. If the user doesn't want the first and last point they can manually trim the output.

Also, I think we should rename DensifyHaversine to HaversineDensify to be consistent with most of our other algorithm variants, but I don't feel that strongly about it.

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

michaelkirk commented 1 month ago

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 commented 1 month ago

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) {}