gis-ops / docker-valhalla

This is our flexible Docker repository for the Valhalla routing engine
MIT License
228 stars 69 forks source link

Road transit times using default configuration #135

Closed johndporter closed 5 months ago

johndporter commented 5 months ago

I am using the default docker image and a tile set for the uk. The transit times for routes are very different from those of tomtom and google with transit times 50% shorter when using my valhalla docker container (eg 346 vs 530secs) . Routing generally seems to be correct and all distances are very similar. This is on routes which do not have any traffic delays, but do have speed limits.

Using the valhalla server at https://valhalla1.openstreetmap.de/route returns similar results to tomtom and google. Is it using a different configuration for road speeds? I can't find any way of looking at the configuration for that server.

nilsnolde commented 5 months ago

Jep it does, you can try with this: https://github.com/OpenStreetMapSpeeds/schema/blob/master/default_speeds.json. Download the json and set the path in mjolnir.default_speeds_config. That could be automated, happy to review a PR to do just that.

johndporter commented 5 months ago

I have tried using the full default_speeds.json above and also a cut down version with very low speeds. Each time I rebuild the tiles, but nothing seems to change the transit time. There is a comment in the valhalla tile build log saying

Enabled default speeds assignment from config: /custom_files/default_speeds.json

The same request still gives different results in my docker image and the https://valhalla1.openstreetmap.de server.

I assume there must still be something different between my local valhalla.json and that on the server? This is my valhalla.json: valhalla.json

nilsnolde commented 5 months ago

Log looks good and you should have it in your tiles now. One way to tell if it really worked is to look inside the JSON, identify a certain road class and its supposed maxspeed, then hit one of those roads with /locate and look at the speed_limit somewhere in there. It should match the JSON value.

I assume you did build with build_admins=True? That's required for the JSON to work. It's not the default, though it should be.. We're moving towards making it a requirement to even run the service: https://github.com/valhalla/valhalla/pull/4469. Too many unexpected things happening if that db isn't built and there's really no reason not to..

johndporter commented 5 months ago

Simply adding the mjolnir.default_speeds_config setting to valhalla.json actually works.

The problem was that something wasn't getting rebuilt with force_rebuild=True in the build config. Explicitly deleting the valhalla_tiles/ and valhalla_tiles.tar from the custom_files directory and then rebuilding fixes the problem.

build_admins=True doesn't seem necessary.

nilsnolde commented 5 months ago

If you didn't build the admins sqlite, it can't work. It's needed to map the right ISO codes from the edges to the speeds JSON. Anyways, happy it seemed to have (somehow) worked for you. In the meantime @chrstnbwnkl has raised a PR to deal with automatically:) https://github.com/gis-ops/docker-valhalla/pull/136

johndporter commented 5 months ago

It is now working for me, but just to check what is happening, I ran the process again with a clean directory.

  1. run with default config -> gives very low transit times
  2. modified valhalla.json to add default_speeds - rebuild by re-running docker-compose -> still gives very low transit times
  3. delete vallhalla_tiles and valhalla_tiles.tar - rebuild -> now gives correct transit times

I don't know exactly how this is expected to work, but it is interesting that something is presumably not getting re-built without deleting the tiles explicitly. The default docker-compose.yml has build_admins=False set, I don't understand what that really does, but it doesn't seem to be required either, the log mentions that admins.sqlite not found, but the transit times are now correct......

chrstnbwnkl commented 5 months ago

This is expected behavior. If you want to force a new graph build, you can pass force_rebuild=True instead of deleting the tiles yourself. The default is False, so it will just fall back to the existing tiles

johndporter commented 5 months ago

Sorry, I should have said. I have tried with force_rebuild=True and it doesn't seem to make any difference to the effect. I still need to explicitly delete the valhalla_tiles to get the behaviour to change.

It seems to be the valhalla_tiles.tar file which makes the difference....

nilsnolde commented 5 months ago

That might very well be. I think there's a slight bug where that forcing will rebuild the tile dir but not remove the tar, so currently you'll have to force the "build_tar" as wel

johndporter commented 5 months ago

Is there another issue, that triggering the rebuild of the tiles (because something changes like the config file or presumably the pbf file) and this does not cause the tar file to rebuild. This seems to have been happening as well.