trekking: add whether there is a usable cycleway in the isbike bike hint

[Totorrr]

Some cycleways are not always properly detected, because some tagging schemes are not as straightforward as having bicycle=yes defined on the way. The isbike bike hint we use would be closer to reality, and thus offering better routing, if we'd consider some more complicated tagging schemes, like "is there a usable cycleway on this road?".

This patch is an attempt to do so with a single block of code. This has the benefit of not messing too much with the existing code, with the downside of not being perfectly integrated within the profile.

[afischerdev]

The build process delivers an error:

RoutingEngineTest > overrideParam FAILED
    java.lang.AssertionError at RoutingEngineTest.java:52

Please try a gradlew clean build and follow the output to find the reason.

[Totorrr]

Hello @afischerdev,

thank you for your comment.

I have been trying to setup the build environment with gradle on my end, but I stopped trying because for each error I corrected when launched the build, I got a new one, and that was starting to take much more time than I could afford!

So, I focused on the error from the automated build instead.

First, it says See the report at: file:///home/runner/work/brouter/brouter/brouter-core/build/reports/tests/test/index.html, do you know if I can get that file from the automated build?

Then as the error is:

RoutingEngineTest > overrideParam FAILED
java.lang.AssertionError at RoutingEngineTest.java:52

Here is the test method:

  @Test
  public void overrideParam() {
    RoutingContext rctx = new RoutingContext();
    rctx.keyValues = new HashMap<>();
    rctx.keyValues.put("avoid_unsafe", "1.0");
    String msg = calcRoute(8.723037, 50.000491, 8.712737, 50.002899, "paramTrack", rctx);
    Assert.assertNull("routing failed: " + msg, msg);

    File trackFile = new File(workingDir, "paramTrack1.gpx");
    trackFile.deleteOnExit();
    Assert.assertTrue("result content mismatch", trackFile.exists());
  }

Line 52 matches the last instruction (using trackFile.exists()).

As the name of the file name paramTrack1.gpx suggests with the 1 suffix, I suppose the test expects an alternative way GPX file to appear when the avoid_unsafe parameter is set. I guess this is because the file https://github.com/abrensch/brouter/blob/master/brouter-core/src/test/resources/paramTrack0.gpx already exists in the working directory when the test is run.

However, when I test the trekking profile from master and the trekking profile from this PR in brouter-web using the same from and to points as in the test, I get the same route result every time with the two trekking profile versions, even for original, 1st, 2nd and 3rd alternatives, and even when I enable avoid_unsafe. The behaviour in this routing case is the same for both profiles.

Also, I noticed none of the calculated routes from my tests are the same as the paramTrack0.gpx route mentioned above. They all differ from it in some way. Now, I don't know if the test set used an up to date map or not, as that may change some routing decisions...

This is paramTrack0.gpx in yellow:

And this is the computed route, original, with avoid_unsafe enabled:

At this point, I may try to modify this PR, reverting my edits, just to re-run the tests to see if the problem happens with an untouched master...

Can you help me understand what is going on here? That would be appreciated!

EDIT: I tried to change a few things in the PR, but the test still fails. :(

[Totorrr]

@zod could you give me a quick hand and explain how this test works (as you set it up), so that I can get why it fails here? 🙏

EDIT: Even changing paramTrack0.gpx (using brouter-web export) to try using the default trekking route as @afischerdev suggests in this comment #641 (comment) doesn't seem to make the test pass. I will thus remove that change and force push back to normal PR state without having resolved the issue...

I just don't get it...

[afischerdev]

I had the same problems when testing with this profile - - without inspecting the source.

In my eyes the test tries to verify if BRouter is able to generate an alternative route.
What do you think about changing the test so that both variants are generated within the test? Something like this:

    String msg = calcRoute(8.723037, 50.000491, 8.712737, 50.002899, "xparamTrack", rctx);
    msg = calcRoute(8.723037, 50.000491, 8.712737, 50.002899, "xparamTrack", rctx);
    Assert.assertNull("routing failed: " + msg, msg);

    File trackFile = new File(workingDir, "xparamTrack0.gpx");
    trackFile.deleteOnExit();
    trackFile = new File(workingDir, "xparamTrack1.gpx");
    trackFile.deleteOnExit();

A different route will be generated than shown above, but the test is valid if another route is found.

[Totorrr]

In my eyes the test tries to verify if BRouter is able to generate an alternative route. What do you think about changing the test so that both variants are generated within the test? Something like this:

[...]

A different route will be generated than shown above, but the test is valid if another route is found.

That sounds OK to me! I may try it here, or you can go first, because I am not sure I can have a look at it this week.

EDIT: ok, I just changed the test as you suggested.

[Totorrr]

@afischerdev Tests are working now!

You / mainteners may consider merging 😛.

Do you want an issue for each PR?

[afischerdev]

@Totorrr

you / mainteners may consider merging 😛.

Let's wait a little longer, maybe someone else would like to comment.

Do you want an issue for each PR?

No, not for me.

Sorry, one more comment:
to be closer to the actual routing please add an extra context to the second run, e.g.

    // 1st alternative
    rctx = new RoutingContext();
    rctx.keyValues = new HashMap<>();
    rctx.keyValues.put("avoid_unsafe", "1.0");
    msg = calcRoute(8.723037, 50.000491, 8.712737, 50.002899, "paramTrack", rctx);

[Totorrr]

This means to me:

* do a first routing
* add a parameter and get a new route

New route in this context is not an alternative route - so the file name is added by zero. You get an alternative route when you reroute with the same values - points and parameters.

@afischerdev So I did some tests :

• when computing the route twice, but adding explicitly avoid_unsafe=0.0 (equivalent to the default value) the second time, I do get an original and an alternative route result (as if no parameter changed),
• when adding avoid_unsafe=1.0 instead, the second time, as this parameter changes the computed route, the 1st result gpx file is overwritten with with a new route. That proves that the parameter was actually taken into account for routing decisions.

As I see it, the test purpose is to ensure that the parameter overriding is indeed working. So I changed the test code accordingly :

• compute the "default route",
• set avoid_unsafe=1.0 and compute the route with same from/to again,
• ensure that an alternative track was not created (which would mean our parameter change did not result in new routing decisions for this from/to couple).

I will now rebase/change the commit history to make it more clean.

[Totorrr]

On lcn/rcn/ncn, I see in taginfo lcn=yes is 218.817 times mapped on ways, ncn 3.933 times and rcn 20.485 times but in my home country lcn=yes is only mapped 18 times and these seems to be in Germany.

In Germany, the tag is present over 92000 times. Indeed, other European countries have it much less often.

Cycleroutes are nowadays tagged via relations so this looks to me like some legacy tagging and looking at the 139 ways that have bicycle=no + lcn=yes I doubt if we should have lcn=yes override things like highway=footway (59) and ways with also foot=no (29). I vote to remove lcn=yes from the isbike condition.

Yes, and we could consider cycleroutes relations for isbike, but I suppose those relations may be lagging behind for weeks or months when ways are updated (constructions, or other access changes), and I think less reliable than the tagging on ways themselves.

When you wrote that the lcn tag would override highway=footway, are you referring to this line?

assign probablyGood = or ispaved and ( or isbike highway=footway ) not isunpaved

I don't see/understand a specific problem here.

About removing lcn=yes, I am hesitating because it is used in some places, and a lot in Germany (about 43% of the world's lcn tags on ways are there) . And I think it does not hurt to have it as it still gives a hint on the bicycle friendly aspect of the road.

Any other opinion is welcomed!

[zod]

@zod could you give me a quick hand and explain how this test works (as you set it up), so that I can get why it fails here? 🙏

EDIT: Even changing paramTrack0.gpx (using brouter-web export) to try using the default trekking route as @afischerdev suggests in this comment #641 (comment) doesn't seem to make the test pass. I will thus remove that change and force push back to normal PR state without having resolved the issue...

I just don't get it...

Linking to commit and then force pushing your changes leads to links which point nowhere, so I can't really check your changes.

I'm not really proud of the test but I'll explain it anyways. If BRouter finds an existing track (paramTrack0.gpx) with exactly the same result it generates an alternative (paramTrack1.gpx), so this test relies on this behavior. Both tracks were generated using the same parameters. We have an OSM export of a small region to run the tests always with the same data so perhaps adding a new reference using current data (from brouter-web) causes some differences.

I think it's sensible to get rid of the reference file and not rely on this behavior.

[Totorrr]

Linking to commit and then force pushing your changes leads to links which point nowhere, so I can't really check your changes.

Sorry for that! Trying to keep a clean commit history for merging. But also trying to keep the current diff to master meaningful.

Also, thank you for the explanation of the test case.

We have an OSM export of a small region to run the tests always with the same data so perhaps adding a new reference using current data (from brouter-web) causes some differences.

I didn't know this, thanks.

I think it's sensible to get rid of the reference file and not rely on this behavior.

That's exactly how I ended up doing it. Compute a route without the parameter, then with the parameter, and ensure that we did not get an alternative route the second time (which would mean our parameter had no impact on the routing result).

[afischerdev]

To conclude this PR, we should return to the beginning:
The defined file "trekking.brf" produces different output, so this test is broken.
There are two options:

• replace the test file with an other track that fits this trekking.brf
• modify the test to generate the first and second routes with the same parameters.
  both produce an alternative track, so test can pass.
  The second option is better in my opinion, it should cover all future changes to trekking.brf

We had something like this code before:

    // 1st run
    RoutingContext rctx = new RoutingContext();
    rctx.keyValues = new HashMap<>();
    rctx.keyValues.put("avoid_unsafe", "1.0");
    msg = calcRoute(8.723037, 50.000491, 8.712737, 50.002899, "paramTrack", rctx);
    // 1st alternative
    rctx = new RoutingContext();
    rctx.keyValues = new HashMap<>();
    rctx.keyValues.put("avoid_unsafe", "1.0");
    msg = calcRoute(8.723037, 50.000491, 8.712737, 50.002899, "paramTrack", rctx);

    File trackFile = new File(workingDir, "paramTrack0.gpx");
    trackFile.deleteOnExit();
    trackFile = new File(workingDir, "paramTrack1.gpx");
    trackFile.deleteOnExit();
    Assert.assertTrue("result content mismatch", trackFile.exists());

Note: The trekking.brf (from your other PRs) has changed in the meantime. So please check which version you want.

Trying to keep a clean commit history

You could close this and add a new PR that is clean and has passed
gradlew build
on your local machine before check in.

[Totorrr]

Thank you for your answer @afischerdev ! And thanks for merging the other PRs.

* replace the test file with an other track that fits this trekking.brf

* modify the test to generate the first and second routes with the same parameters.
  both produce an alternative track, so test can pass.
  The second option is better in my opinion, it should cover all future changes to trekking.brf

Just like zod (who wrote "I think it's sensible to get rid of the reference file and not rely on this behavior"), I also prefer an option like the second one (removing the need for a static result track file).

However, I don't understand the logic of your proposal.

If we "generate the first and second routes with the same parameters", how can they "both produce an alternative track"? If we do the exact same routing twice, I think that it will produce one route the first time, and an alternative route the second time. Can you please explain what you mean?

Maybe we have a different understanding on the overrideParam() function purpose. From my understanding, its purpose is to check if the overriding of some profile parameter with, for example, rctx.keyValues.put("avoid_unsafe", "1.0"); actually works by comparing the route result with and without the parameter enabled.

• If it doesn't work, that would be equivalent to computing twice the same route with the same parameters and so would compute one "initial" route and one alternative route.
• If it does work, it would compute one initial route and overwrite the former "initial" route on the second run (not computing an alternative, but a different route for a different routing request with different parameters).

In my PR, I check if what I describe in the second point actually happens. Is that not what we want?

You could close this and add a new PR that is clean and has passed gradlew build on your local machine before check in.

Agreed.

The new branch is already ready, I'm just waiting for your answer to my comment above before creating the PR.

[afischerdev]

@Totorrr

If we "generate the first and second routes with the same parameters", how can they "both produce an alternative track"? If we do the exact same routing twice, I think that it will produce one route the first time, and an alternative route the second time.

That may be poor wording, but you understand the situation correctly.

Maybe we have a different understanding on the overrideParam() function purpose. From my understanding, its purpose is to check if the overriding ... actually works by comparing the route result with and without the parameter enabled.

May be bad wordring too. But here, your understanding doesn't match what the test is intended to do.
Your description of changing parameter is the normal routing situation. We don't have to test that for filenames.
The test here routes once and executes the same routing (same parameters) to obtain an alternative. Test this alternative filename xxx1.

[Totorrr]

May be bad wordring too. But here, your understanding doesn't match what the test is intended to do. Your description of changing parameter is the normal routing situation. We don't have to test that for filenames. The test here routes once and executes the same routing (same parameters) to obtain an alternative. Test this alternative filename xxx1.

@afischerdev (or @zod ?) sorry, it is still a bit confused for me, can you just say quickly, what is the purpose or goal of the overrideParam() test function, in your mind?

I mean, it is testing that some functionality (or use case) is working as expected, but which functionality exactly?

[afischerdev]

@Totorrr

If BRouter finds an existing track (paramTrack0.gpx) with exactly the same result it generates an alternative (paramTrack1.gpx), so this test relies on this behavior.

or

The test here routes once and executes the same routing (same parameters) to obtain an alternative.

[Totorrr]

Hello @afischerdev,

please excuse by later answer.

Purpose?

You answer by saying what the test does (how it works). This is not saying what its purpose is.

Example: what is the purpose of this car? It burns fuel and transform it to a rotation movement in a wheel. No, it's purpose is to travel pretty fast while carrying heavy loads, compare to walking.

Here are the purposes I think this test has:

1. it checks if computing a given route actually works,
2. it checks if overriding a parameter actually works (has an actual impact on the routing decisions). The function name overrideParam() suggests this.

You suggest

• to generate the first and second routes with the same parameters.
  both produce an alternative track, so test can pass

=> This only work for purpose 1.

I suggest (option A):

1. generate first route with default parameters,
2. generate second route with overriden parameters,
3. ensure that the first route result was overriden by the the second route result (no generation of an alternative route).

=> I think this works for purpose 1 and 2.

Downside: we don't know if generating an alternative route would work. This could be done by doing step 2 again after step 3 and check if an alternative is generated (option B).

Question

Do you agree to go for option A or B?

Thanks!

[afischerdev]

@Totorrr
My answer is B, with only two steps.

Feel free to change the function name or add a description.
But please, don't change the test.
The only difference from the original test is that we no longer have a paramTrack0.gpx file, but have to generate it first (same points, same parameters).

Use something like the above code.
And please test this locally before publishing.

[Totorrr]

Hello @afischerdev,

I fail to see how option B can be realised with 2 steps, and how the above code ensures that purpose 2 is fulfilled.

That being said, I created a new PR #852, in which I use the code you mention, with a few added comments.

What do you think about merging that one, and closing this one?

Thanks!

[afischerdev]

@Totorrr

I fail to see how option B can be realised with 2 step

I was focused to this statement: "... This could be done by doing step 2 again after step 3 ..." and wanted to point out that the first step (generation with default parameters) is not necessary.

What do you think about merging that one, and closing this one?

Yes, that is the way.

[Totorrr]

@afischerdev

I [...] wanted to point out that the first step (generation with default parameters) is not necessary.

In the code you mentioned, just to check my hypothesis, I just replaced this:

rctx.keyValues.put("avoid_unsafe", "1.0");

by that:

rctx.keyValues.put("peanut_chocolate", "1.0");

on both lines, and the test is successful. I think it demonstrates that purpose 2 is not fulfilled by this code.

[afischerdev]

@Totorrr
Yes, that is correct.
But now no parameter is overwritten. Or do you have in your new trekking.brf a parameter named peanut_chocolate?

[Totorrr]

@afischerdev

do you have in your new trekking.brf a parameter named peanut_chocolate?

I don't.

I'm just saying that purpose 2 is not fulfilled by this code. Even if I do have a parameter named peanut_chocolate in my profile. I do believe purpose 2 would be nice to have.

That being said, I vote for merging this as is, and for discussing further improvements of this test function in another PR.

[afischerdev]

Ok, that is a fine idea.
Thanks for cooperation.