-
Notifications
You must be signed in to change notification settings - Fork 30
Merge with origin #157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Merge with origin #157
Conversation
|
The commit should fix #148 , adding IGN Geoservices directions and isochrones. |
|
Thank you! I'll start reviewing it very soon, hoping to merge it within the week if all goes well! |
mthh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, CI jobs are failing because the poetry.lock file is no longer synchronized with the pyproject.toml file.
Once you have seen my previous comment (about the need to write the poetry dependency on the one hand, and about the dependencies on responses on the other), could you push an updated version of this poetry.lock file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pyproject/poetry changes!
I left a few additional comments before merging.
In addition, you should add an entry in the documentation, in the docs/index.rst file (between HereMaps and MapboxOSRM).
IGN
---
.. autoclass:: routingpy.routers.IGN
:members:
.. automethod:: __init__| resource "bdtopo-osrm" also supports "exceptionnal". | ||
| :param resource: The routing resource to use. Should be one of | ||
| "bdtopo-osrm", "bdtopo-pgr", "bdtopo-valhalla", "pgr_sgl_r100_all" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the “pgr_sgl_r100_all” resource, I don't see it listed in the documentation on https://geoservices.ign.fr/documentation/services/services-geoplateforme/itineraire. When I tried to calculate a route with this resource, it told me that it didn't exist, and when I do a getCapabilities, I see “pgr_sgl_r100_all_test_IGN_4” but not “pgr_sgl_r100_all.” If we are to believe the documentation, I think it would be enough to keep the first three resources listed (but perhaps you had a use for “pgr_sgl_r100_all” on your end?).
| raw=response, | ||
| ) | ||
|
|
||
| def isochrones( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IGN isochrones API only allows a single interval value to be passed (correct me if I'm wrong!), but nothing happens if the user passes a list or tuple of multiple values in the intervals argument (and only the first one is silently taken into account).
In my opinion, something needs to be improved here, either in the type hints (but the IGN.isochrones method will not have the same signature as the other {router}.isochrones functions), or in the code (to raise an error, or at least a warning, if multiple values are passed).
Another option could be to make the various necessary requests (if the user passes the values [600, 1200, 1800], in the intervals argument, then we would make three requests and assemble the whole thing in an Isochrones object). In theory, this seems good from a user perspective, but we shouldn't overuse the API (which is said to be limited to 5 requests per second... so we could limit “intervals” to a list/tuple of 5 values max?).
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the following should be added on line 67 for the documentation of the get_router_by_name function:
:class:`routingpy.routers.graphhopper.IGN`
No description provided.