-
Notifications
You must be signed in to change notification settings - Fork 29
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
added vroom support #47
base: master
Are you sure you want to change the base?
Conversation
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.
looks good @chrstnbwnkl . it's fine for now to stay as close to vroom api as you did for the object model.
just a couple of things regarding type hinting need to be fixed. I generally prefer the way to put type hints directly into the def signature when you're defining the arguments. here is the old way, also fine and maybe even better for very long type hints:
https://github.com/gis-ops/routing-py/blob/185bf65acffca7f16c179291388065245c2f791f/routingpy/routers/valhalla.py#L173
Good point re type hinting, I thought it might be best to leave it in the rst type docstrings, since that's the way this has previously been dealt with in routingpy, but I completely agree that we should use proper type hinting instead! |
you can run
|
And you also can add your own time/distance matrix like here, otherwise you'll have to bother with proper router configuration |
you can only do that bcs you have vroom locally installed. the vast majority doesn't. then we'd have to package |
This PR adds support for Vroom's optimization by
Vroom
router class with anoptimization
methodroutingpy.optimization.py
)Tests are still missing at this point.