Rhumb no longer goes past pole

[joe-saronic]

• 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.

---

Fixes #1152

[apendleton]

I don't have review powers in this repo, but my two cents: one result of this change is that the rhumb-line intermediate and intermediate-fill operations might now panic given some float inputs. I think that's probably undesirable/surprising, and we'd be better off either with propagating this API change across to those methods as well, or clamping (either just for those or in general). You could consider returning an Option for intermediate, mirroring the change for destination, but I think for intermediate-fill, returning a Vec<Option<T>> will probably make the results sort of tedious to work with in practice, and I wonder if consumers might prefer to just have them clamped? And if so, if it wouldn't be better for consistency to just always clamp.

(I suppose another option would be Option<Vec<T>>, and just None the whole thing if any of them would have been None, which seems easier to use but strictly less useful.)

[joe-saronic]

I am happy to add checks that both inputs are between -90 and 90. There is no other scenario that will make any of the methods fail that I'm aware of.

[apendleton]

For sure, agreed that that's the circumstance where it matters. I think implementation isn't that important (do we check up front for -90 < x < 90 or do we just wait until the destination operation returns None and do something in that case; I think the latter is probably easier?). The main question is what happens in that case from an API perspective, and I think it probably shouldn't be "panic," it should be one of:

• we always return a float but it's clamped
• we return an Option<T> or Vec<Option<T>> or something, and return None in this case, or
• We return a Result<T, E> or Vec<Result<T, E>>and return anErr` case if out-of-bounds inputs are passed in

[urschrei]

For sure, agreed that that's the circumstance where it matters. I think implementation isn't that important (do we check up front for -90 < x < 90 or do we just wait until the destination operation returns None and do something in that case; I think the latter is probably easier?). The main question is what happens in that case from an API perspective, and I think it probably shouldn't be "panic," it should be one of:

• we always return a float but it's clamped
• we return an Option<T> or Vec<Option<T>> or something, and return None in this case, or
• We return a Result<T, E> or Vec<Result<T, E>>and return anErr` case if out-of-bounds inputs are passed in

Happy to assign you once you accept the org invite!

[joe-saronic]

I attempted to add clamping, but realized that it may be impractical. I left the broken commit with a unit test that illustrates the issue in. Basically, clamping to exactly +/-90 will make the actual point invalid and the intermediate longitudes will (correctly) be set to NaN. The alternatives are adding/subtracting epsilon to the clamp, which I think is a bad idea, or returning an Option<T> (Option<Vec<T>>, not Vec<Option<T>>, since all the longitudes will be invalid in that case). Given that an out-of-bounds value indicates user error, I think that refusing to do the computation is totally fine in this case.

Another option might be to have RhumbCalculations::new return Option<Self> rather than Self since that's really where the error is caught. Since distances are finite even pole-to-pole, constructing a RhumbCalculations must be allowable at least over that range.

[apendleton]

@joe-saronic sorry for the delay. Yes, I think you make a good case that clamping doesn't make sense here.

I think that refusing to do the computation is totally fine in this case

That makes sense, but I don't think the mechanism for refusing should be to panic. It's user error, but needn't be unrecoverable user error. I think Result or Option would be more appropriate here. And yeah, Option<Vec<T>> seems reasonable rather than Vec<Option<T>>.

@urschrei accepted the invite, thanks!

[joe-saronic]

@apendleton I will update the PR shortly.

[michaelkirk]

Sorry that this sat here so long.

As I understand this PR, it's trying to validate that its input is in a valid range. Helping people do the right thing is good, but optionals tend to be contagious and can suddenly make a lot of API's more verbose, so there's a cost associated with changing them.

We have a similar issue with Haversine - which assumes LngLat points, but does nothing to validate that they are actually valid.

In the meanwhile, many of the line_measure traits have been reformulated under a set of traits in the line_measures module. So one issue is that these changes need to be rebased, and we'd likely need to make some changes to the traits, which are currently infallible. If I were going to pursue that, I'd start by adding an associated type Output to Destination trait, so that some implementations can return an Option. Like I said, Optional's tend to be contagious, so this might have some farther reaching changes.

Another option, would be to leave the "unchecked" apis as they are, and have a higher level more verbose API for people who want the algorithm to validate their input, and have some kind of method that does is_valid_params(params) { return Some(Rhumb.destination(a,b,c)) } else { None }

Somewhat relatedly, I never ported VincentyDistance to the new line_measures regime in part because it's fallible. I suspect it's similarly possible by parameterizing an output type, but due to inter-trait dependencies, there may be knock-on effects that require updating more than just one trait.

I'm not confident enough that this would be a net win to invest my own time in hammering out a prototype, but if someone else wants to give it a go, I would review.