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

GeodesicBearing produces NAN when y value is outside of [-90, 90] #1059

Open
JosiahParry opened this issue Aug 25, 2023 · 6 comments
Open

GeodesicBearing produces NAN when y value is outside of [-90, 90] #1059

JosiahParry opened this issue Aug 25, 2023 · 6 comments

Comments

@JosiahParry
Copy link
Contributor

When using geodesic_bearing() method with a point where the latitude is outside of the coordinate range [-90, 90], the result will be NaN. However, there is no corresponding result when a longitude value falls outside of the range [-180, 180]. I understand that values that fall outside of these ranges are likely to be planar coordinates and are not intended to work. If that is the case, I suspect the result of this method should return Option<f64> instead of f64 to better handle these cases.

On the contrary, the haversine_bearing() method has no such NaN behavior. It will return an f64 for any value provided despite it only being intended for values that fall within the [-180, 180] x range and [-90, 90] y range.

Below are reproducible examples:

use geo::{point, GeodesicBearing};

#[test]
fn y_out_of_positive_range() {

    // y is out of range of Lat vals will return NaN
    let x = point!( x: 0_f64 , y: 91_f64);
    let y = point!( x: 0_f64 , y: 0_f64);

    assert!(x.geodesic_bearing(y).is_nan())
}

#[test]
fn y_out_of_negative_range() {

    // y is out of range of Lat vals will return NaN
    let x = point!( x: 0_f64 , y: -91_f64);
    let y = point!( x: 0_f64 , y: 0_f64);

    assert!(x.geodesic_bearing(y).is_nan())
}

#[test]
fn x_out_of_range() {
    // x is out of long rannge but will compute fine
    let x = point!( x: -361_f64 , y: 0_f64);
    let y = point!( x: 0_f64 , y: 0_f64);

    assert!(!x.geodesic_bearing(y).is_nan())
}
// running 3 tests
// test y_out_of_negative_range ... ok
// test y_out_of_positive_range ... ok
// test x_out_of_range ... ok
@JosiahParry
Copy link
Contributor Author

Note that this is using geo v 0.26.0

@michaelkirk
Copy link
Member

This third one actually seems reasonable to me:

#[test]
fn x_out_of_range() {
    // x is out of long rannge but will compute fine
    let x = point!( x: -361_f64 , y: 0_f64);
    let y = point!( x: 0_f64 , y: 0_f64);

    assert!(!x.geodesic_bearing(y).is_nan())
}

Longitude is just being wrapped.

e.g.

#[test]
fn x_out_of_range() {
    // x is out of long rannge but will compute fine
    let x = point!( x: -361_f64 , y: 0_f64);
    let y = point!( x: 0_f64 , y: 0_f64);

    assert!(!x.geodesic_bearing(y).is_nan())

    let x_simplified = point!( x: -1_f64 , y: 0_f64);
    assert_eq!(x.geodesic_bearing(y), x_simplified.geodesic_bearing(y))
}

That results hold true for haversine and geodesic bearing, which seems arguably reasonable to me. Do you think anything should change about that example?

The first two, with latitude out of bounds, seem more problematic - how should we handle nonsense input?

geodesic_bearing is returning f64::NaN, which is probably mostly an artifact of being ported from c++, but at least it indicates "something is wrong here". In rust a Result<f64> type would probably be more idiomatic.

But with the haversine implementation - it seems like it's just garbage output right? Is there any world where you can get a meaningful result from a latitude more than 90º?

My inclination at this point is, for both HaversineBearing and GeodesicBearing:

  1. return an Err if the input latitude is out of bounds.
  2. proceed as normal if the longitude is out of bounds, knowing that the math will effectively mod 360 the results.

Changing the return type would be a breaking change.

@urschrei
Copy link
Member

I'll be conservative and say out of bounds long should also be an Err as it might indicate problems with the input data – it's better to draw attention to the problem there and then than rely on the fact that it happens to wrap correctly imo.

@JosiahParry
Copy link
Contributor Author

HaversineBearing and GeodesicBearing recently superceded Bearing as a breaking change. I think returning Result<f64> would be an appropriate api change and within a short enough time period to be "stabilized." I would agree with @urschrei here—return an Err even though the calculation can continue but probably shouldn't.

@cojmeister
Copy link

cojmeister commented Mar 12, 2024

Hey, can this be assigned to me?

  • Checking input parameters for lat lon
  • Returning a Result<f64, Err>

Should I make a custom Err?

@cojmeister
Copy link

Hey, I'm stuck and I need some help!
I opened PR #1161
If you could take a look and give me a hand, I would really appreciate it.

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

4 participants