Error handling in shortest_path calculation for multiple origins/destinations

[a-slice-of-py]

Problem description

• What did you do?
  • Submitted a request to OSMnx' shortest path with a single node as origin but multiple destinations (or viceversa).
• What did you expect to happen?
  • Receive an informative error message which points out the mismatch between types of origin(s) and destination(s).
• And what actually happened instead?
  • Received TypeError: object of type 'int' has no len()

Environment information

• What operating system are you using?
  • Windows 10 Enterprise, v1909 build 18363.1556
• What Python version are you using?
  • Python 3.8.8
• What OSMnx version are you using?
  • OSMnx 1.1.1

Provide a complete minimal reproducible example

import osmnx as ox
# graph from sample address
G = ox.graph_from_address('Via Montenotte, 18, Savona, Italy')
# single node as origin (without wrapping into a list with a single element)
orig = list(G.nodes)[0]
# multiple destinations (list with two nodes)
dest = list(G.nodes)[1:3]
# shortest path with mismatch between orig and dest types
# (dest is iterable, being a list, while orig isn't)
ox.shortest_path(G, orig, dest)

A possible improvement for handling such situations could consist in replace line from 524 to 531 in osmnx/distance.py with the following

if not (hasattr(orig, "__iter__") or hasattr(dest, "__iter__")):
    # if neither orig nor dest is iterable, just return the shortest path
    return _single_shortest_path(G, orig, dest, weight)
else:
    # if both orig and dest are iterables
    if (hasattr(orig, "__iter__") and hasattr(dest, "__iter__")):
        # if orig/dest are iterable, ensure they have same lengths <- before, this was taken for granted
        if len(orig) != len(dest):  # pragma: no cover
            raise ValueError("orig and dest must contain same number of elements")
    # if exactly one between orig and dest isn't iterable
    else:
        raise ValueError("orig and dest must be both iterables")

which provides a more informative error message.

[gboeing]

I suppose the fix could be either 1) improving the documentation to explicitly state something like "orig and dest must either both be iterable or neither must be iterable" or 2) making the error message clearer, as you state. It looks like the else-if in your code snippet could be simplified with an elif.

[a-slice-of-py]

I would prefer something like solution 2), but I get we're talking about details.

Regarding implementation, you're right - I was trying to modify current version as little as possible. Something like this should do the job

def shortest_path(G, orig, dest, weight="length", cpus=1):
    """
    ...
    """
    # if neither orig nor dest is iterable, just return the shortest path
    if not (hasattr(orig, "__iter__") or hasattr(dest, "__iter__")):        
        return _single_shortest_path(G, orig, dest, weight)
    # if both orig and dest are iterables
    elif (hasattr(orig, "__iter__") and hasattr(dest, "__iter__")):
        # ensure they have same lengths
        if len(orig) != len(dest):  # pragma: no cover
            raise ValueError("orig and dest must contain same number of elements")

        if cpus is None:
            cpus = mp.cpu_count()
        utils.log(f"Solving {len(orig)} paths with {cpus} CPUs...")

        if cpus == 1:
            # if single-threading, calculate each shortest path one at a time
            paths = [_single_shortest_path(G, o, d, weight) for o, d in zip(orig, dest)]
        else:
            # if multi-threading, calculate shortest paths in parallel
            args = ((G, o, d, weight) for o, d in zip(orig, dest))
            pool = mp.Pool(cpus)
            sma = pool.starmap_async(_single_shortest_path, args)
            paths = sma.get()
            pool.close()
            pool.join()

        return paths
    # if exactly one between orig and dest isn't iterable
    else:
        raise ValueError("orig and dest must either both be iterable or neither must be iterable")

[gboeing]

Would you like to contribute this as a PR?

[a-slice-of-py]

Hi @gboeing, sorry for the late response, I've had tough weeks.

Anyway, glad to see that you merged the suggestion in the code base. Thanks!