Rhumb no longer goes past pole

[joe-saronic]

• [x] I agree to follow the project's code of conduct.

• [x] 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> orVec<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> orVec<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.