genthalili / searoute-py

A python package to calculate the shortest sea route between two points on Earth.
Apache License 2.0
63 stars 14 forks source link

`searoute.searoute` returns Exception when one of the coordinates is zero. #22

Closed vinisalazar closed 5 months ago

vinisalazar commented 7 months ago

Hi,

I'm running searoute v1.2.2. One of the coordinate points in my dataset is at the Equator, and has a registered latitude of 0.0. I noticed that the searoute.searoute function would return the following Exception when I tried calculating the distance from that point to another:

Exception: Origin/Destination must not be empty or None

A toy example can be executed using Null Island:

In [1]: import searoute as sr

In [2]: sr.searoute((0.0, 0.0), (1.0, 1.0))
---------------------------------------------------------------------------
Exception                                 Traceback (most recent call last)
Cell In[2], line 1
----> 1 sr.searoute((0.0, 0.0), (1.0, 1.0))

File /data/scratch/projects/punim1293/vini/.conda/envs/data/lib/python3.11/site-packages/searoute/searoute.py:40, in searoute(origin, destination, units, speed_knot, append_orig_dest, restrictions, include_ports, port_params, M, P)
     17 """
     18 Calculates the shortest sea route between two points on Earth.
     19 
   (...)
     36 a Feature (geojson) of a LineString of sea route with parameters : `unit` and `length`, `duration_hours` or port details
     37 """
     39 if not origin or not all(origin):
---> 40     raise Exception('Origin/Destination must not be empty or None')
     42 if not destination or not all(destination):
     43     raise Exception('Origin/Destination must not be empty or None')

Exception: Origin/Destination must not be empty or None

This check on the origin and destination variables, although elegant, is flawed. It would be better off if replaced for something else, e.g.:

def _validate_point(p):

    # Check if the point has exactly two elements
    if len(p) != 2:
        return False

    # Check if both elements are numeric (integer or float)
    if not all(isinstance(val, (int, float)) for val in p):
        return False

    # Check if none of the values is None
    if None in p:
        return False

    return True

If you are inclined, I would be happy to submit a PR fixing that for your appreciation.

Thank you, Vini

genthalili commented 5 months ago

Hi @vinisalazar ,

Thanks for reporting this issue and suggesting the code. I let the lat between -90 and 90 deg ( should help users who are confused while setting as lat, lon instead of lon,lat : #23 ), and didn't put any limitation on lon, as it can be any float number (this can be improved by normalising it between -180 and 180 deg, maybe in a next version).

I just published version v.1.2.3 which improved the way a coordination is validated. It should be fixed now, let me know if you still face the issue.

genthalili commented 5 months ago

Considering as fixed in version >=1.2.3 Feel free to re-open a new issue if the error persists.