mapado / haversine

Calculate the distance between 2 points on Earth
MIT License
330 stars 62 forks source link

haversine raises an exception above 180 degrees latitude #49

Closed uri-rodberg closed 2 years ago

uri-rodberg commented 2 years ago

Hi,

I defined London as (51.5072, -0.1276), and then I had 3 methods to define the other side of the earth from London:

(51.5072 * -1.0, -0.1276 + 180.0)
(51.5072 - 180.0, -0.1276)
(51.5072 + 180.0, -0.1276)

Then I tried to check the distance between each point, and the distance to London. When I defined the other side of the earth from London as (51.5072 + 180.0, -0.1276), haversine raises an exception when I check its distance to (51.5072 * -1.0, -0.1276 + 180.0) (which should be 0). You have a problem with angles above 180 degrees. I know latitude and longitude shouldn't usually be above 180 degrees (actually latitude should be from -90 to 90 degrees), but it would be nice if haversine would be able to handle other degrees too while converting them to the relevant degree (51.5072 - 180.0 and 51.5072 + 180.0 should be the same degree).

Notice that 51.5072 - 180.0 is -128.4928 and it works, even though it's below -90 degrees (it's just a way to define the point on the other side of the earth).

uri-rodberg commented 2 years ago

Maybe you can normalize values by converting them to the relevant numbers. For example, convert 270 degrees to -90, convert 720 to 0 etc.

uri-rodberg commented 2 years ago

Also, the limits of (latitude, longitude) is not documented except that they should be "in decimal degrees".

jdeniau commented 2 years ago

Hi @uri-rodberg ,

Thanks for opening the issue. Would you be OK to open a PR with a fix ?

uri-rodberg commented 2 years ago

Hi @jdeniau,

OK, I'll try. How do you want me to fix this? Do you want me to convert the input to a specific range? And what range of values do you expect the input to be?

For example, you can define latitude to be from -90.0 to 90.0, and longitude to be from -180.0 to 180.0. Do you want to define them this way?

uri-rodberg commented 2 years ago

Hi,

I read on Google the ranges of latitude and longitude:

Latitude and longitude are a pair of numbers (coordinates) used to describe a position on the plane of a geographic coordinate system. The numbers are in decimal degrees format and range from -90 to 90 for latitude and -180 to 180 for longitude.

Now, I would like to know if you receive input beyond this range, what do you want to do? I think maybe it makes sense to raise an exception if the latitude is below -90 or above 90, but convert the longitude to the required range by adding or subtracting 360 multiplied by the relevant integer. But I'm not sure. Maybe raise an exception anyway? Or convert the latitude too? In theory you can add or subtract 360 for the latitude too, and if it's not between -90 and 90, reverse the location (add 180 degrees to the longitude). But I'm not sure what's the correct thing to do. What do you think?

jdeniau commented 2 years ago

A conversion to a range seems to work fine, but be I do not work with haversine a lot, so I might not choose the best option.

@MuellerSeb , @merschformann , may I ask for your opinion another time ? (And please tell me if you don't want to be notified anymore ! πŸ˜ƒ )

merschformann commented 2 years ago

And please tell me if you don't want to be notified anymore !

All fine. :hugs:

I tried to wrap my head around it by replaying the example given by @uri-rodberg :

When I defined the other side of the earth from London as (51.5072 + 180.0, -0.1276), haversine raises an exception when I check its distance to (51.5072 * -1.0, -0.1276 + 180.0) (which should be 0)

When not "normalizing" the points are

London: (-51.5072, 179.8724)
Other: (231.5072, -0.1276)

which indeed causes a math domain error, since d in the haversine formula approaches 0 from the wrong side - the negative one (which is not a good value to get the sqrt of). I am not fully sure whether this is exclusively a numerical problem or a problem of the out of bounds input values. :thinking:

When "normalizing" the points to [-90,90] and [-180,180] the points are

London: (-51.5072, 179.87239999999997)
Other: (51.50720000000001, -0.12760000000000105)

and the distance is 20015.114442035923. I think this distance is correct and it should not be 0, as we can see in the lat difference of the normalized points (specifically + vs -).

However, this is where discussions may start and I am not fully certain that I have sufficient knowledge about expectations of the GIS community (and who else uses this library) to have a good recommendation. It seems to me there are at least three options though:

  1. Always apply the following formula I used for normalization:

    lat1, lat2 = (lat1 + 90) % 180 - 90, (lat2 + 90) % 180 - 90
    lng1, lng2 = (lng1 + 180) % 360 - 180, (lng2 + 180) % 360 - 180
  2. Throw an exception if lat not in [-90,90] or lon not in [-180,180]

  3. Apply a different normalization that is more canonical than the one I used above

I added the code I wrote for testing to #52 just in case we want option 1. I am not certain this is the right way though. Some more feedback would be recommended, I think.

uri-rodberg commented 2 years ago

Hi @merschformann,

I think you have a mistake. London is (51.5072, -0.1276) which is +51.5072, not -51.5072. And it's true that I would expect the distance from London to (51.5072 + 180.0, -0.1276)==(231.5072, -0.1276) to be 20015.114442035923. And therefore, 231.5072 + 180.0, -0.1276) is equivalent to (51.5072 * -1.0, -0.1276 + 180.0)==(-51.5072, 179.8724). So if you want to "normalize" lat1, lng1 then you take lat1=(lat1 + 90) % 360 - 90, and then if lat1>90 then lat1=180-lat1 and lng1-=180 (or +=180) and then lng1=(lng1 + 180) % 360 - 180. So in this case you take "the other side of the earth" both for lat1 and lng1. This makes sense for me.

That being said, I think maybe it makes more sense to raise an exception if lat1 is not in the range -90 to 90, and then you can optionally assign lng1=(lng1 + 180) % 360 - 180 or raise an exception too. Because it doesn't matter if lng1 is any number as long as you convert it to -180 to 180, but I'm not sure it would make sense to convert lat1 to -90 to 90 if it's not in this range.

Another option is to define a setting that will determine if to normalize the latitude and longitude, or raise an exception.

merschformann commented 2 years ago

@uri-rodberg : I think I misunderstood your problematic case due to the way I intuitively normalized. And my choice of variable names was poor too. :sweat_smile:

So, I think we are getting to the same discussion of what is the correct way of normalizing. Simply "normalizing" latitude to [-90,90] like I did may indeed be confusing when thinking about the full circle. Your strategy seems a bit complicated to me too, but it may well be the way to go. Maybe we can have some further opinions?

Let me update #52 anyway to accommodate my improved understanding.

uri-rodberg commented 2 years ago

Hi @merschformann,

Just to emphasize why calculating % 180 is incorrect: The north pole is lat 90. If you receive lat 89.9 it's close to the north pole. Lat 90.1 should be close to the north pole too, but % 180 makes it close to the south pole (-90), which I don't think is what the user intended. If you calculate 180 - 90.1 you receive 89.9 which is close to the north pole.

jdeniau commented 2 years ago

I understand now why you propose to throw an exception only for latitude and normalize longitude πŸ˜„

It seems that longitude out of bounds is an error and should throw.

For the latitude, I like consistency, so I would suggest to throw an exception too, but if having a "weird" latitude is a thing that happen, we can normalize it. As you prefer.

merschformann commented 2 years ago

Regarding the normalization: I noticed that too. The way @uri-rodberg proposes probably makes sense. The one I intuitively used certainly leads to unexpected results on the user side.

Personally, I would suggest throwing an exception for non-proper lat/lons and accepting an optional normalize parameter that applies @uri-rodberg normalization instead of raising the exception (if true).

Edit: I also updated #52. Maybe this is what we want?

jdeniau commented 2 years ago

Fixed by #52