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

Customizable metric space for line measures #1298

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

michaelkirk
Copy link
Member

@michaelkirk michaelkirk commented Jan 11, 2025

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Motivation

Currently, our Haversine metric space calculation uses the IUGG's "mean radius", but there are several other reasonable values. Similarly, the Geodesic internally uses Karney's geographic algorithms which support other geoid parameters.

Proposal

By reorganizing our traits, we can let users take advantage of all the associated algorithms (length, densify, distance) for any given (potentially customizable) metric space.

The trick is, almost nobody (by my count) actually wants this configurability, so the resultant API must not make it burdensome for the vast majority who want the "default" case.

Some conversation in #1274 and another in #1140 lead me to revisit the recently overhauled line_measure traits, and that it's possible to support this kind of customization in a reasonable way.

There are a lot of changes, but they are pretty mechanical. You can get a pretty good idea of the kinds of changes our users will need to make by reviewing this diff.

As well as being more expressive, I actually prefer where the code is now, without the turbo-fish. I just wish I'd avoided the churn by thinking of it before the big changes in the v0.29.0 (primarily #1216) were released (on 2024-10-30):

// Before v0.29.0)
p1.haversine_distance(p2);
line_string.haversine_length();
line_string.densify_haversine(100);

// Currently (as of v0.29.0)
Haversine::distance(p1, p2);
line_string.length::<Haversine>();
line_string.densify::<Haversine>(100);

// After (this PR)
Haversine.distance(p1, p2);
Haversine.length(&line_string);
Haversine.densify(&line_string, 100);

// and now you can do this:
let planet_mars = CustomHaversine::new(3_389_500.0);
planet_mars.densify(&line_string, 100);

References

This solves the same problem as #1220, but more generally.
Tracking issue for line_measure overhaul here #1181

In itself, this adds no new functionality, but sets us up for
configurable line measures - e.g. a Haversine with a custom earth
radius (or non-earth radius!) or a non wgs84 Geodesic
"long ago" `Densify` was defined on the individual geometries: LineString, Line, etc.

Recently, these methods were changed to be generic so they could work
with Euclidean vs. Haversine, etc.

However, to support configurable metric spaces (e.g. Haversine with
custom planet radius) a "type" generic is not enough - we need to pass
a (potentially configured) metric space as an argument.

However, this made the arguments a little awkward:

    line.densify(&Haversine, 100)

Passing `&Haversine` is a little awkward, so this commits switch to
instead be:

    Haversine.densify(&line, 100)

Which allows us to seemlessly add support for something like:

   let mars_radius = 3_389_500
   let mars = CustomHaversine::with_radius(mars_radius);
   mars.densify(&line, 100)
@@ -2,7 +2,6 @@

## Unreleased

- Fix page location of citation for mean earth radius used in Haversine calculations
- Implement `RTreeObject` for `Triangle`.
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've only moved this to geo/CHANGES.md where I think it belongs.

@urschrei
Copy link
Member

Haven't reviewed, but this is a lot better from a UX perspective, and – I think – worth the churn.

@michaelkirk michaelkirk marked this pull request as draft January 14, 2025 19:06
@michaelkirk
Copy link
Member Author

michaelkirk commented Jan 14, 2025

Converting this to a draft while I incorporate some feedback from @lnicola - what do you think, does
Make all Haversine customizable, removing CustomHaversine make things better or worse?

In summary:

// "long ago" before 0.29.0
point1.haversine_distance(point2)
point1.rhumb_distance(point2)

// as of 0.29.0
Haversine::distance(point1, point2)
Rhumb::distance(point1, point2)

// v1 of this PR:
CustomHaversine::new(3_389_500.0).distance(point1, point2)
Haversine.distance(point1, point2)
Rhumb.distance(point1, point2)

// current version of this PR (with @lnicola's suggestion - removing "CustomHaversine")
Haversine::new(3_389_500.0).distance(point1, point2)
HAVERSINE.distance(point_1, point_2)
RHUMB.distance(point_1, point_2)

@michaelkirk michaelkirk force-pushed the mkirk/customizable-metric-space branch from 71038e0 to 0dcb842 Compare January 16, 2025 23:28
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 this pull request may close these issues.

2 participants