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

Update the trekking profile #641

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

Conversation

afischerdev
Copy link
Collaborator

Here a new version for the trekking profile suggested by @EssBee59

@afischerdev afischerdev temporarily deployed to BRouter October 28, 2023 12:23 — with GitHub Actions Inactive
@zod
Copy link
Collaborator

zod commented Oct 28, 2023

It seems like the updated profile does no longer have a avoid_unsafe parameter. The fix for the overrideParam testcase should be a valid parameter (which produces a different result) instead of changing the file.

@afischerdev
Copy link
Collaborator Author

@zod
thanks for your review. The trekking profile has an avoid_unsafe, but it is located a little bit down.

  1. override param in 'RoutingEngineTest' paramTrack comes out as paramTrack0 (first alternative, can't say why the first test was against paramTrack1, don't see a second call)
  2. override param in 'RouteServerTest' also works for me ('avoid_unsafe=0' brings a different results)

@afischerdev
Copy link
Collaborator Author

@zod
I made new tests to get this.
First I found no paramTrack0.gpx in my local repo. Must have dropped it But it is needed to build the paramTrack1.gpx
Then I generated the route with old and new profile.

test_alt
test_neu

First one is the old situation - blue the unsafe route, red the safe result and green the test route from repo
Second one is new - blue unsafe, red the safe route.
As you can see the new safe route is near to the test route. That seems to be the reason that there is no paramTrack1.gpx as result

What to do:
I would like to replace the old test route0 with the unsafe route. Both profiles produce it in the same way.
What do you think?

@quaelnix
Copy link
Collaborator

This PR adds the following without comment:

  • railwaypenalty (which will affect routing)
  • barrierpenalty (which will affect routing)
  • uphillcost penalty of 80 (which will affect routing)
  • consider_traffic logic that does not fit a trekking profile
  • surface=compacted to isunpaved (which will affect routing)
  • downhillcutoff and upillcutoff logic that avoids steep inclines
  • avoid_path we previously agreed on to not add: Fix profile regressions #593
  • smoothnesspenalty we previously agreed on to not add: Seriously consider surface and smoothness data #58

removes the following without comment:

  • stick_to_cycleroutes logic

changes the following without comment:

  • ignore_cycleroutes from false to true
  • tracktype penalty weights

And introduces a new logic for subtracting costfactors that can result in the costfactor becoming less than 1, which not only has the potential to completely destroy routing, but also contradicts what is currently being discussed here: #610.


If this is merged, I will end the collaboration.

@rkflx
Copy link
Collaborator

rkflx commented Oct 30, 2023

This PR [ adds | removes | changes ] the following without comment

It's hardly the first time this has been criticized, yet nothing changed.

If this is merged, I will end the collaboration.

FWIW, I already decided some time ago to stop caring about and commenting on such PRs, so do not mistake silence for agreement with any such PR. I suspect other contributors quit or reduced their involvement out of frustration how things are run here, too.

@EssBee59
Copy link
Collaborator

Hello !
Yes, I suggested this profile version to replace the current "trekking", the PR is a kind of a misunderstanding with afischerdev.
Anyway, (perhaps better so!) we have now first reactions to the proposed changes, so that a real discussion on the parameters could start?

My first goal was to change the default value for "ignore_cycleroute":
see #622
the further changes are not so critical, but as example not considering surface=compacted in the same way as fine_gravel is for me a severe bug.
Yes, I accept the critic that no documentation was provided, thank Quelnix for your list!

I suggest to start (or better to continue) the discussion with #622
Till now I am missing arguments to not change the default value?

I hope we can prepare together a solution to this first parameter and go on with the others.
Regards

@afischerdev
Copy link
Collaborator Author

@zod
Added a new paramTrack file and went back to old test. But for me it is still more a test for the generation of a 2nd route then a test for a param change.

@afischerdev afischerdev temporarily deployed to BRouter October 30, 2023 17:33 — with GitHub Actions Inactive
@afischerdev
Copy link
Collaborator Author

@quaelnix
You're right, a detailed explanation would have been good.
When I started I was still unsure whether I should create an issue or a PR. But I decided to do a PR because we already have enough issues about it that end without a result.
But I didn't ask for any further information about it. Sorry.

From my point of view, it would be good if we have a person responsible for the profiles who would bring the issues together. That's why I had the feeling that no one felt responsible for it and nothing was happening. I think it was good that @EssBee59 had the courage to create a new version of the trekking profile.

With a maintainer we could also go back to the normal way, create an issue and a PR for a result.

If this is merged, I will end the collaboration.

Hard words, but understandable.
I would miss you. We need a corrective, someone who will point out the critical points and encourage changes. But I know that is not always easy going.

@EssBee59
Copy link
Collaborator

EssBee59 commented Nov 6, 2023

With a maintainer we could also go back to the normal way, create an issue and a PR for a result.

Yes, with a maintainer defect #622 would probably not hang for weeks!!!

It had be better to start with that, but now I found time to document the changes suggested in this PR profile:

1-cycle_route

Not every part of a cycle-route is a "safe harbor":
Constrains exist to create a cycle-route, as example

  • it should start and end in the center of a town
  • big groupes of cyclist are expected, so that it has to avoid narrow pathes or cycleways!

Statistics from Hesse-Germany show as example, that a big part of the OSM primary (10%) and secondary (20%) ways are used in these cycle-routes, most of them without any cycleway=lane!

In the trekking profile (with the default parameters) these ways get a lower cost as any other ways (track, cycleway etc...), so that the resulting routing is not safe and do not reflect what the Brouter can do!

2-surface=compacted

Fine_gravel ==> cost 1
compacted ==> cost 2

But compacted (quality just below asphalt!) is better than fine_gravel

The profile make a very limited usage of the surface tag
... assign ispaved = surface=paved|asphalt|concrete|paving_stones|sett
... assign isunpaved = not or surface= or ispaved surface=fine_gravel|cobblestone

3-uphillcost penalty

The current implementation in the trekking:

assign downhillcost = 60 # %downhillcost% | Cost for going downhill | number
assign downhillcutoff = 1.5 # %downhillcutoff% | Gradients below this value in percents are not counted. | number
assign uphillcost = 0 # %uphillcost% | Cost for going uphill | number
assign uphillcutoff = 1.5 # %uphillcutoff% | Gradients below this value in percents are not counted. | number

assign downhillcost = if consider_elevation then downhillcost else 0
assign uphillcost = if consider_elevation then uphillcost else 0

With the default value "consider_elevation=true", uphillcost remains =0, and this is a very poor usage of the "elevation" fonction as supported by the Brouter.

example
2 routes are possible to the same destination:

Route A:
20 km with uphill factor of 7.5

  • 100 km with a downhill factor of 1.5

Alternativ Route B:
125 Km with uphill=downhill=0

The user would certainly prefer "Route B" as he choosed "consider_elevation".... but Brouter will suggest "Route A" because no elevation costs are added during his calculation.
(because uphillcost=0 and donwhillcutoff = 1.5)

4- downhillcutoff and upillcutoff logic that avoids steep inclines

Also when "consider_elevation" is changed to "false", bikers are not able to use ways with very steep inclines.
To avoid such ways, a minor change is possible:

assign downhillcost_ = 60 # %downhillcost_% | Cost for going downhill | number
assign downhillcutoff = switch consider_elevation 1.5 10
assign uphillcost_ = 80 # %uphillcost_% | Cost for going uphill | number
assign uphillcutoff = switch consider_elevation 1.5 10

5- consider_traffic

Default value is false, so that the traffic do not impact the routing.
The pseudo-tag "estimatd_traffic_class" is now available world-wide for primary, secondary and tertiary highways, it should be considered for biker routing.
(impact can be changed with the parameter, current setting "very low traffic" can be discussed)

6- smoothnesspenalty

The surface tag is important, but the surface quality can very different between 2 highways with the same type and surface:

  • surface is gravel (tracktype2), but very very bad quality ...or excellent
  • surface of a cycleway is asphalt, but with smoothness=horrible ... and some monthes later after renovation the smoothness get "excellent"

When available, the smoothness is a good help!

Remark:
When the tag is considered in the profile, then the users have the posibility by need to add or change its value in OSM!!!
So, it impacts positively not only the own routing, but it is also a benefit for all OSM users.

7- railway and barrier penalties

OSM delivers detailed tags about railways and barriers, why not to add penalties when the cyclist have to pass them?

assign railwaypenalty switch railway=level_crossing 155 0

Note: each rail-track is tagged, so the penalty depends on the number of rail-tracks to be crossed-and probably on the number of trains crossing that place per hour/day.

assign barrierpenalty switch barrier= 0
switch barrier=block|bollard 140

Note: Barriers not only cost time, the risk of accidents increases.

Here a new version containing these changes:
trekking_v11.brf.txt

But weaknesses remain (a.e. surface tag), and the maintenance remains difficult.
Better would be a new, well structured, easy to maintain profile!

Regards

@zod
Copy link
Collaborator

zod commented Nov 7, 2023

@zod Added a new paramTrack file and went back to old test. But for me it is still more a test for the generation of a 2nd route then a test for a param change.

I agree that the tests is far from ideal, but unfortunately I didn't find a better way to test if param overriding works. BRouter only creates the 2nd GPX file when the track differs from the existing track. If you would pass avoid_unsafe=0 in the overrideParam test it would fail.

@afischerdev
Copy link
Collaborator Author

@zod
You don't have a better idea either. And if we test both aspects, it's ok too.

@quaelnix
Copy link
Collaborator

quaelnix commented Nov 9, 2023

Yes, with a maintainer defect #622 would probably not hang for weeks!!!

Your assumption of being smarter than the person who created the trekking profile leads to ill-conceived changes and I am simply not going to accept them. The logic you are trying to remove does exactly the right thing despite map errors.

I have already proven that the alleged routing error in the first example you gave in this issue ticket is caused by incorrect map data, and here is the proof that it is no different in the second example:
b486
Source: https://www.google.de/maps/@49.9787262,8.5809106,3a,60.3y,274.65h,88.89t/data=!3m6!1e1!3m4!1s79wZe-swSnTwfujOZnYEbw!2e0!7i16384!8i8192?entry=ttu

@EssBee59
Copy link
Collaborator

@quaelnix,

thanks for the kind words!
It takes too much time for me to read comments that are confusing or completely out of place.
From now on I will simply ignore your posts.

@ other participants of this PR:

in the first example Quelnix suspects a mapping error: the cycle_route would not use the secondary hw as mapped, but the parallel path!
(mapping error)

No proof is delivered for this!
I rather think, the route was planed for groups of cyclists biking together "sunday mornings" where the traffic remains low. And the path is this case probably not prefered to the secondary hw. (avoid path for groups)

in the second example Quaelnix delivers a picture taken nearly 1 km outside the route, strange for a proof!
Within the critical route part, no path exists parallel to the primary hw.

Is it possible, that 10 % of the primary and 20% of the secondary hw in Hess are related BY ERRORS to cycle_route?

@afischerdev
Copy link
Collaborator Author

Let us stay by facts. Please.

Routing is very individual and many answers are correct.

Change that appear to be critical:

- assign   ignore_cycleroutes       = false  # %ignore_cycleroutes% | Set true for better elevation results | boolean
+ assign   ignore_cycleroutes       = true   # %ignore_cycleroutes% | Set to false to favor the segments on cycleroutes | boolean

At first glance I would say it doesn't matter what the starting value is.
But cycle routes can present a few problems for routing:

  • as shown in Trekking profile and cycle-routes #622, routes can run on roads even though there is a bike path. This doesn't have to be a mistake, routes can be designed for tourist reasons or for sporting reasons (I don't know if there are any rules for that, didn't find one).
  • the route relations is not always adjusted if access=no, highway=construction or something similar is suddenly set on a way. There are some issues in that direction
  • more personally: normally, when I see a cycle path is available, but the route continues on the road, I decide to use the cycleway. But 'intelligent' apps with re-routing push me back onto the road. Annoying and with the potential to miss the right descent.

@EssBee59

  1. Please tell us how did you find the errors on Hessen data?
  2. When I copy the new profile to BRouter-Web profile box and select ignore_cycleroutes=false, it didn't follow the route (using first sample of Trekking profile and cycle-routes #622).

May be a feature 'follow cycleroute' could help for trekking profile that follows the direction of a route but uses the saver way. When I use the original profile with ignore_cycleroutes=false and avoid_unsafe=true, route doesn't change.

@quaelnix
Copy link
Collaborator

quaelnix commented Nov 12, 2023

From now on I will simply ignore your posts.

We can reopen this pull request once you change your mind.

No proof is delivered for this!

radroutenplaner-hessen
osm-relation

Within the critical route part, no path exists parallel to the primary hw.

Within the "critical route part" the speed limit of primary highway is 50 km/h or less and your alternative looks like this:
alternative-route
Source: https://www.google.de/maps/@49.9880073,8.5751062,3a,75y,311.2h,80.22t/data=!3m6!1e1!3m4!1seaC8bC_-mAKskLhcHiBTNQ!2e0!7i16384!8i8192?entry=ttu

@quaelnix quaelnix closed this Nov 12, 2023
@afischerdev
Copy link
Collaborator Author

Here we are again.

@quaelnix
I don't think it's a good action to just close.
As your pictures show, action is needed.

We can

  • enable avoid_unsave to work in the original profile
  • in @EssBee59 's suggestion recall to follow the routes

Or maybe someone else would like to contribute a completely new trekking profile from their pool.

@quaelnix
Copy link
Collaborator

quaelnix commented Nov 13, 2023

I don't think it's a good action to just close.

None of the proposed changes is acceptable in its current form and as long as @EssBee59 is neither willing to properly test his changes nor to discuss them, this pull request is not going to happen. His behavior is completely unacceptable. Period.

enable avoid_unsave to work in the original profile

Yes, of course we can discuss if there are ways to improve the avoid_unsafe switch. But this has nothing to do with this pull request. And by the way a highway=primary section with maxspeed=40, right of way, asphalt in pristine condition and basically no hgv traffic is not unsafe.

As your pictures show, action is needed.

Yes, someone needs to put the route relation were it actually belongs and properly tag the path that has not been touched in 10 years and make this path the combined foot and cycleway it actually is.

@EssBee59
Copy link
Collaborator

Hello afficherdev,

  1. Please tell us how did you find the errors on Hessen data?

The statistics were created using overpass turbo
http://overpass-turbo.eu/

here some examples:
overpass.txt

A further example...

http://brouter.de/brouter-web/#map=9/51.0552/13.7219/osm-mapnik-german_style&lonlats=8.76663,50.017366;13.725973,51.044692

I tested with the current profile, the current profile + ignore_Cycleway=true and with the trekking_v11 including the changes suggested above

image

@quaelnix
Copy link
Collaborator

quaelnix commented Nov 13, 2023

Even with this relaxed query: https://overpass-turbo.eu/s/1zDU, the rate of false positives is extremly high:

-> Map is wrong.

-> Map is incomplete. Route relation again on the wrong highway ...


I guess we don't have to talk about how the situation will be in other countries, where the density of mappers is much much lower than in Germany and where the trekking profile is also supposed to produce artefact free routes that will not get you stuck on paths where you have to carry your bike ...

@quaelnix
Copy link
Collaborator

assign smoothnesspenalty =
  switch smoothness=intermediate            0
  switch smoothness=bad                     0.5
  switch smoothness=very_bad                1
  switch smoothness=horrible		    3
                                            0 

The result of this will be that you downgrade ways which are tagged as smoothness=bad (of which almost all are perfectly fine for trekking bikes) and instead favor any of the ~ 97 % alternative ways that do not carry any smoothness tag.

This is plain stupid and results in a near 100 % failure rate:

@quaelnix
Copy link
Collaborator

but as example not considering surface=compacted in the same way as fine_gravel is for me a severe bug.

Another example of a gross misunderstanding of how the trekking profile works. Adding compacted to this exclusion list actually removes such ways from the list of unpaved ways, which throws off the tracktype cost logic and results in chosing:

@afischerdev
Copy link
Collaborator Author

@quaelnix

...as long as @EssBee59 is neither willing to properly test his changes nor to discuss them...

I think it's sentences like this that make @EssBee59 a little more reserved:

Your assumption of being smarter than the person who created the trekking profile leads to ill-conceived changes...

Anyway, thanks for your samples, good basis for a discussion.

  • Map errors
    Yes, we don't have to talk about that. But we need a few considerations about how we get from the street to the cycleway.
    As remark only: In the first example I didn't find any route networks
    And in the second example, both have this information.
  • smoothnesspenalty
    Yes, I've seen the table for that and bad is good enough for trekking bike.
  • compacted
    Yes, that breaks the tracktype. But also cobblestone as unpaved is not really understandable when I see sett as parved. (present in both profiles)

@quaelnix
Copy link
Collaborator

I think it's sentences like this that make @EssBee59 a little more reserved:

His complete inability to follow arguments is not new and now it is enough. Not to mention this .txt mess, broken markdown formatting in basically every post and the aparrent refusal to open his own pull requests. Again, I am not going to accept this anymore.

Yes, we don't have to talk about that.

We need to talk about this because the design goal of the trekking profile was (and still is, in my opinion) to make it resistant to map errors.

In the first example I didn't find any route networks

What do you mean by that? The relation in question does carry a network=lcn tag.

But also cobblestone as unpaved is not really understandable

cobblestone is excluded from the list of unpaved roads!

@afischerdev
Copy link
Collaborator Author

@quaelnix

Yes, we don't have to talk about that.

We need to talk about this ...

This quote has other context, Original it was:

  • Map errors
    Yes, we don't have to talk about that.

And first sample of your map error post is:

B3: https://www.openstreetmap.org/way/364301602

And I've found: it is member of the route Radnetz Stadt Darmstadt

cobblestone is excluded from the list of unpaved roads!

Yes, my fail - short reading only. But then remark is the other way round, why cobblestone and fine_gravel are not in ispaved collection. Historical I guess. But I'm digressing from the topic.

And a last remark on compacted. I found this

Best sort of ways below paving with asphalt, concrete, paving stones.

Seems to be better in surface hierarchy then fine gravel.


His complete inability to follow arguments is not new and now it is enough. Not to mention this .txt mess, broken markdown formatting in basically every post and the aparrent refusal to open his own pull requests. Again, I am not going to accept this anymore.

Not everyone is born with developer skills.
And I think we have to accept collaborators who are not that equipped. And not to push them out. They often come from an interested user community and contribute here the discussion of the scene.
Our discussions are not primarily user driven. That's okay, we can do important things of our own. But it also makes us forget the user needs we have. E.g. if you take the example route Frankfurt - Dresden, around 500 km, it takes a lot of time to calculate.
And I am willing to make the technical transfer for @EssBee59 if needed. I haven't been able to do this so well in the past.

@quaelnix
Copy link
Collaborator

This quote has other context

Yes, sorry, my bad.

Historical I guess.

No, the (almost exclusive) purpose of these two variables is to feed the probablyGood logic, that can be considered a whitelist of ways that are likely kind of decent.

If you start feeding this whitelist with things that might not be good, you're taking it ad absurdum and end up with the routing artefacts I showed above.

You simply cannot look at one line of a profile and say this looks incorrect, lets change it. Doing it like that will almost always fail horribly.

And I think we have to accept collaborators who are not that equipped.

To be honest, I don't know what to say anymore. If by now @EssBee59 is still not able to grasp the idea behind the cycleroute logic, then yes, maybe this is the end of the discussion. One last example (from: #479):

E.g. if you take the example route Frankfurt - Dresden, around 500 km, it takes a lot of time to calculate.

I agree that we should try our best to keep the performance of the trekking profile as high as possible.

@afischerdev
Copy link
Collaborator Author

I added a little test for the variables inside a profile.
And a minimal profile with the logic on our discussion: probablyGood is defined as true also when we have no surface definition. Because some said this is a cycle way.
But if we have an undefined value (e.g bricks, is added in the next generation lookups.dat) isunpaved becomes true.
You could use this for short results:

$ gradlew brouter-expressions:test

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

Successfully merging this pull request may close these issues.

5 participants