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

Replace HaversineIntermediate with HaversineLineInterpolatePoint and HaversineDensify #1181

Open
michaelkirk opened this issue May 17, 2024 · 2 comments

Comments

@michaelkirk
Copy link
Member

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

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

No branches or pull requests

2 participants