search

nilsnolde / routingpy

🌎 Python library to access all public routing, isochrones and matrix APIs in a consistent manner.
https://routingpy.readthedocs.io/en/latest/?badge=latest
Apache License 2.0
272 stars 28 forks source link

MapboxOSRM isochrone costing parameters #21

Closed RemaniTinpo closed 3 years ago

RemaniTinpo commented 3 years ago

Hi and thanks for you nice package,

Here's what I did

Query for an isochrone polygon using the class MapboxOSRM as client.

image

Here's what I got

RouterApiError: 422 ({'message': 'Invalid query param', 'code': 'InvalidInput'})

Here's what I was expecting

An object of type routingpy.isochrone.Isochrones

Here's what I think could be improved

  1. I think the issue came from the 'costing' parameters. https://github.com/gis-ops/routing-py/blob/801bb5b3f80bc4852d7a30e0741d5ff5970a52bd/routingpy/routers/mapbox_osrm.py#L401 When I dry run, then I copy and paste the url, remove the costing parameters and curl the url it works well.

  2. I think there is a typo in the readme, on the multiprovider example. The profile is 'cycling' and it supposes to be mapbox/cycling for the isochrone query. https://github.com/gis-ops/routing-py/blob/801bb5b3f80bc4852d7a30e0741d5ff5970a52bd/routingpy/routers/mapbox_osrm.py#L369

nilsnolde commented 3 years ago

Thanks for the clear issue report. Seems to be a copy/paste fail from the Valhalla module, definitely a bug. Do you wanna give it a shot in a PR? It's pretty quickly done, you just have to remove the costing stuff and adapt the tests accordingly.

If you don't have the time to PR, can you please provide the code as code not as a pic? Then it's easier for us to reproduce..

BTW, you might as well use the MapboxValhalla connector, it's the exact same. It might look like "OSRM" but it's really Valhalla doing the isochrone calculations at Mapbox.

RemaniTinpo commented 3 years ago

Thanks @nilsnolde for the answer, I pushed a PR.