Skip to content
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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

added vroom support #47

wants to merge 9 commits into from

Conversation

chrstnbwnkl
Copy link
Member

@chrstnbwnkl chrstnbwnkl commented Nov 2, 2021

This PR adds support for Vroom's optimization by

  • adding a new Vroom router class with an optimization method
  • porting objects from Vroom's API to Python classes (in routingpy.optimization.py)

Tests are still missing at this point.

routingpy/optimization.py Outdated Show resolved Hide resolved
@nilsnolde nilsnolde self-requested a review November 3, 2021 11:52
Copy link
Member

@nilsnolde nilsnolde left a 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

@chrstnbwnkl
Copy link
Member Author

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!

@nurikk
Copy link
Contributor

nurikk commented Apr 28, 2022

you can run vroom binary locally, without relying on crappy vroom-express api

class VroomException(Exception):
    pass


def solve(problem: vroom_problem.Definition, search_time_limit_ms: int = None) -> vroom_solution.Solution:
    cmd = ['vroom']
    if search_time_limit_ms is not None:
        cmd.append('-l')
        cmd.append(str(int(search_time_limit_ms / 1000)))
    solving_result = subprocess.run(cmd,
                                    input=problem.json(exclude_none=True).encode(),
                                    capture_output=True)
    if solving_result.returncode == 0:
        return vroom_solution.Solution.parse_raw(solving_result.stdout)
    else:
        raise VroomException(solving_result.stderr.decode().strip())

@nurikk
Copy link
Contributor

nurikk commented Apr 28, 2022

And you also can add your own time/distance matrix like here, otherwise you'll have to bother with proper router configuration

https://github.com/VROOM-Project/vroom-scripts/blob/2bd8337dacd18c9773c458559a381ce300d4fb13/src/utils/matrix.py#L33

@nilsnolde
Copy link
Member

you can run vroom binary locally

you can only do that bcs you have vroom locally installed. the vast majority doesn't. then we'd have to package vroom for every platform which is out-of-scope here. routing-py is all about HTTP. if you want local you should use a package that's designed for that, e.g. pyvroom or our pyvalhalla for local valhalla

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants