mapado / haversine

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

last release is breaking my CI #54

Closed 12rambau closed 2 years ago

12rambau commented 2 years ago

I'm using haversine for a while now and I really like it's simplicity. My today's latest build failed because you introduced something new in the lib: https://github.com/12rambau/sepal_ui/runs/7278765322?check_suite_focus=true

def_ensure_lat_lon(lat: float, lon: float):
"""
    Ensure that the given latitude and longitude have proper values. An exception is raised if they are not.
    """
if lat < -90or lat > 90:
>           raiseValueError(f"Latitude {lat} is out of range [-90, 90]")

it should be <= and >= right ?

jdeniau commented 2 years ago

Hi @12rambau,

you are totally right! I will push a fix right now.

You CI will probably continue to fail though as your latitude seems to be -180, and latitude should be between -90 and 90

12rambau commented 2 years ago

wait I think I was too fast publishing an issue.

you're raising ValueError when values are strictly outside of the bounds which is exactly what we want, so the issue is somewhere else. did you inverted the variable order somewhere ? (I'm always getting lost as there is no standart on lat/lng or lng/lat)

jdeniau commented 2 years ago

OK I was about to respond the same thing. The code seems correct, but your latitude is -180, where latitude should be between [-90, 90]

The Equator has a latitude of 0°, the North Pole has a latitude of 90° North (written 90° N or +90°), and the South Pole has a latitude of 90° South (written 90° S or −90°) wikipedia

12rambau commented 2 years ago

that's it I checked my code and effectively when nothing is added I'm using the full map zoom level in the Google Earth Engine convention which is lng/lat...

So nice choice to add this check in the lib ;-) I'll correct the bug from my lib and close this issue once it works

jdeniau commented 2 years ago

OK so you do swapped latitude and longitude in your code, did I understand that correctly ?

12rambau commented 2 years ago

yep and as it was not tested before, my CI was going full straight ahead without complaining

12rambau commented 2 years ago

solved from my side in https://github.com/12rambau/sepal_ui/pull/535/commits/06d2de8e855e2030a1eb90d7b6a2cfae6d6a01ae

sorry for the noise

jdeniau commented 2 years ago

No problem, it's nice to have comment about haversine usage !