remove mapbox valhalla #114
Conversation
| def __init__( | ||
| self, | ||
| base_url: str, | ||
| api_key: Optional[str] = None, |
There was a problem hiding this comment.
I had kept this line in OTP to maintain compatibility with the compare_providers notebook. We can either keep it or delete it in OTP and update the notebook code.
There was a problem hiding this comment.
Didn't realize that while reviewing, thanks for the headsup. OTP is not in the compare_providers notebook right? Even if, yeah, we should update the notebook in that case to make it optional. We shouldn't have unused function arguments just for the sake of running examples. Maybe you can update it in the current PR?
routingpy/routers/valhalla.py
Outdated
| @@ -31,10 +31,11 @@ | |||
| class Valhalla: | |||
| """Performs requests to a Valhalla instance.""" | |||
|
|
|||
| _DEFAULT_BASE_URL = "https://valhalla1.openstreetmap.de" | |||
|
|
|||
| def __init__( | |||
| self, | |||
| base_url: str, | |||
There was a problem hiding this comment.
Missing default for base_url
| @@ -10,9 +10,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. | |||
|
|
|||
| ### Added | |||
| - Valhalla `/expansion` examples in the Jupyter Notebook | |||
| - OpenTripPlanner v2 support for routing & isochrones | |||
There was a problem hiding this comment.
I wonder if it wouldn't be better to specify the supported version for each router. Not in the class name, but in a parameter.
| "openrouteservice": ORS, | ||
| "opentripplanner": OpenTripPlannerV2, |
There was a problem hiding this comment.
Do we really need several aliases? I find it more bearable over time to provide the right key from the start.
There was a problem hiding this comment.
We don't need it, but typing "otp" instead of "opentripplanner_v2" is nicer IMO. The particular line you're indicating is indeed a bit more useless
There was a problem hiding this comment.
Yes, I agree with you about the abbreviations. I was thinking more of the version with underscore or hyphen. But no biggie, it was just a detail.
| def __init__( | ||
| self, | ||
| base_url: str, | ||
| api_key: Optional[str] = None, |
| @@ -513,6 +505,7 @@ def matrix( | |||
| avoid_locations: Optional[List[List[float]]] = None, | |||
| avoid_polygons: Optional[List[List[List[float]]]] = None, | |||
| units: Optional[str] = None, | |||
| date_time: Optional[dict] = None, | |||
There was a problem hiding this comment.
Ahah yes I had come across it too, it was in my TODO thank you. As a 2nd step, I'd like to have 2 input parameters for all routers: departure_datetime and arrival_datetime instead, both of which are object datetime aware.
There was a problem hiding this comment.
It wasn't supported in Valhalla until ~ 2-3 months ago: valhalla/valhalla#4071.
I agree it'd be nicer to have a more generic way and have Valhalla convert that to its own request format. Until OTP there wasn't any OSS router supporting that, I guess that's why we didn't really think about it.
|
I've been too slow 😅 |
|
But my comments were non-blocking. It was more of a discussion 😉 |
fixes #95