Remove tertiary highways from preferred and increase speed on footways

[caspg]

Right now, GH preffers tertiary roads over footways with bicycle=yes tags for bike profile.
This leads to not opimial routing, especially when those footways are connecting cycleways.

@ratrun suggested that we should remove tertiary from preffered highways and bump speed in the footway (only with bicycle=yes) #3014

Example 1

Before:

After:

Example 2

Before:

After:

[caspg]

I updated tests but not sure what to do with this one: https://github.com/graphhopper/graphhopper/blob/master/core/src/test/java/com/graphhopper/GraphHopperTest.java#L464

We probably need to find new example location?

[karussell]

Thanks for this fix and the examples.

We probably need to find new example location?

Can you import the test file into GH and show a screenshot of this route for master and your PR? I'm not yet sure if this is just cosmetics or if removing tertiary from preferHighwayTags has stronger consequences.

[caspg]

@karussell They look exactly the same. I run it with: They are not the same. I forgot to recompile.

profiles:
    - name: bike
      custom_model_files: [bike.json]

Master branch:

My branch:

[karussell]

This still looks like reasonable alternative.

@ratrun isn't removing tertiary from the preferredTags a bit risky regarding other route choice? And should we test it more intense before merging? And if we do this, shouldn't it be done for all bike priority parsers for consistency reasons?

[ratrun]

This still looks like reasonable alternative.

@ratrun isn't removing tertiary from the preferredTags a bit risky regarding other route choice? And should we test it more intense before merging?

I think that the only thing which might happen is that a tertiary is not taken any longer in favour of longer residential, so it should be OK.

And if we do this, shouldn't it be done for all bike priority parsers for consistency reasons?

I think that you are right for the MountainBikePriorityParser, but we should leave it unchanged for the RacingBikePriorityParser.

[caspg]

Yup, removing tertiary only from MTB and leaving in RacingBike seems reasonable. Will update PR.

[caspg]

@ratrun @karussell How should I change test? Also, in test it returns 1 route (probably without any alternative).
Earlier screenshots comes from running default bike.json custom model.

https://github.com/graphhopper/graphhopper/blob/master/core/src/test/java/com/graphhopper/GraphHopperTest.java#L464

[INFO]
[ERROR] Failures:
[ERROR]   GraphHopperTest.testAlternativeRoutesBike:464 expected: <3> but was: <1>

[karussell]

It is strange that it does not return an alternative in the integration test but returned one in maps (the screenshot you showed). It would be important to have some alternatives in this test - maybe you can achieve this by slightly moving e.g. the destination coordinate towards the other alternative?

Or can you try maps again and use the config and file from the integration test?

[caspg]

@karussell How should my bike.json custom model look to match the test configuration? I run it with default bike.json before.

profiles:
    - name: bike
      custom_model_files: [bike.json]

EDIT:

I tried this config which I guess it's what test is using:

{
  "priority": [
    { "if": "bike_access", "multiply_by": "bike_priority" },
    { "else": "", "multiply_by": "0" }
  ],
  "speed": [
    { "if": "true", "limit_to": "bike_average_speed" }
  ]
}

I also got alternative routes. I run it with:

mvn clean install -DskipTests \
  &&  rm -rf graph-cache \
  && java -Xmx8g -Xms8g -Ddw.graphhopper.datareader.file=./core/files/north-bayreuth.osm.gz -jar web/target/graphhopper-web-*.jar server config-example.yml

[ratrun]

@caspg I could not find out what you are doing differently, but I cannot reproduce your screenshots about the alternatives. Here is my screenshot for the original code. It matches the test with 3 alternatives:

I'm using the checked in bike.json from git.

With your branch there is no alternative found as indicated by the test, but by slightly changing the destination coordinates into 49.982089,11.599224 the three alternatives are found again.

Therefore my suggestion is to change the test into:

        GHRequest req = new GHRequest(50.028917, 11.496506, 49.982089,11.599224).
                setAlgorithm(ALT_ROUTE).setProfile(profile);
        req.putHint("alternative_route.max_paths", 3);
        GHResponse rsp = hopper.route(req);
        assertFalse(rsp.hasErrors(), rsp.getErrors().toString());

        assertEquals(3, rsp.getAll().size());
        // via ramsenthal
        assertEquals(2697, rsp.getAll().get(0).getTime() / 1000);
        // via unterwaiz
        assertEquals(2985, rsp.getAll().get(1).getTime() / 1000);
        // via eselslohe -> theta; BTW: here smaller time as 2nd alternative due to priority influences time order
        assertEquals(2783, rsp.getAll().get(2).getTime() / 1000);

Here is the new screenshot:

[caspg]

@ratrun Thanks! I've just updated the test and PR is ready for review.

[karussell]

Ok, thank you. Let's try this.