Custom rule question regarding relationship to existing flag encoders

I have started to play around with the 3.0 custom routing and I have a basic question which I could not answer myself by reading the source code: The question is why a custom query with an empty ruleset does not lead to the same result compared to the flag encoder result.
To illustrate my question here is the ordinary request for bicycle:

https://graphhopper.com/maps/?point=48.387608%2C15.718861&point=48.352142%2C15.741177&locale=de&elevation=true&profile=bike&use_miles=false&selected_detail=distance&layer=Omniscale

Now I press the “custom button” with the empty rule, this leads to:

Why do we get different results here?

1 Like

Good catch! See the bug here: custom requests doesn't consider `priority` EncodedValue · Issue #2292 · graphhopper/graphhopper · GitHub

So currently we assume all priority changes in the custom model, which is good in theory, but in order to make the migration better (and simpler) we need to provide a solution for this issue. Best solution if we could directly use the bike$priority encoded value like:

"priority": [{
  "if": true,
  "multiply_by": "bike$priority"
}]

(which we could then use on the server-side custom model and so a query to bike would use it by default). But this requires some internal changes. Still it should not be that hard. Also this would be a nice feature for other encoded values.

Thank you for the fast response! Then I would like to do something like this in order to overwrite such a default rule:

"priority": [{
  "if": true,
  "multiply_by": "bike$priority" * 2
}]

This doubles the influence of the priority and a factor of 0.5 would reduce it by 50%.

Thanks for this use case. As my current plan was not to allow (mostly) arbitrary statements like in the if-clause, would it work for you if you could do something like:

"priority": [{
  "if": true,
  "multiply_by": "bike$priority"
}, {
  "if": true,
  "multiply_by": 2
}]

But I’m not sure, maybe it makes sense to allow arbitrary statements there as well.

Or maybe we introduce a new action that would be more meaningful and easier to implement:

"priority": [{
  "set_to": "bike$priority"
}, {
  "if": true,
  "multiply_by": 2
}]

(hm, but this is less powerful)

If I may choose I prefer your second proposual. It “sounds” more logical to me.

I can have a look - maybe even something like this works:

"priority": [{
  "set_to": "bike$priority * 2"
}]

I’ve implemented a prototype in this branch: GitHub - graphhopper/graphhopper at set_to_trial

I think, to match the original bike vehicle with weighting=fastest we need a custom_model like this:

{
 "distance_influence": 0,
 "priority": [{
   "set_to": "0.5 + bike$priority",
   "maximum": 1.5
 }]
}

Notes about this:

  • weighting=fastest uses PriorityWeighting which is a bit confusing. But PriorityWeighting is what we want to compare our CustomWeighting with.
  • The "distance_influence": 0 is required for our custom model when we want to make it behave identical as we do not have a distance factor in PriorityWeighting
  • a maximum value is required in general for the getMinWeight estimate to make A* behave correct (and faster)
  • ugly: a maximum entry is required in set_to although we could easily determine the maximum of the EncodedValue. But we cannot know if the formula is e.g. x or 1/x. We could assume a default maximum of 1 to make it a bit easier but it would not help here.
  • a maximum of 1.5 is required as the best value can be 1 plus the 0.5 (see PriorityWeighting)
  • it is not possible to request a custom_model with a maximum above 1 from the client-side but we can still put this server-side in the custom bike profile (config.yml)
  • we have a bug in CustomWeighting that does not account for the minFactor as we do in PriorityWeighting and need to fix it before we can properly compare both results.

So it is a bit harder than expected to migrate from fastest to custom weighting but doable :slight_smile: and will offer many advantages

Can we not use set_to like the other two operators?

{
  "priority": [{
    "if": "road_class == MOTORWAY",
    "set_to": 0.5 
  }]
}

and then support using encoded values on the right hand side (and also within a numerical expression like 0.5*bike$priority)?

Yes, this might be the cleaner solution although I’m unsure if a conditional set_to will be used often in practise and it would always end up as "if":"true". Another more fundamental change would be to make the conditional expression (if, else-if) optional for all operations (set_to, multiply_by, limit_to)

Not sure if this is an actual problem but as then the value can be a string or a number it might be a problem for the API docs or clients.

Furthermore for the client-side I see no way around the additional maximum entry as we use LM or require at least a follow up limit_to?

Really? What about

{
  "speed": [
     {
       "if": "road_class == SECONDARY",
       "set_to": 66
     }
   ]
}

To me it seems like a rather obvious use-case to set explicit speeds depending on some condition, like certain road_classes or areas.

What would be the advantage of this?

I probably don’t understand the problem yet, but for the other operators we already have this check on the server-side: We evaluate one statement after the other and if the resulting speed exceeds the max speed we reject the custom model. Also e.g. for server-side custom models we do not have this LM max restriction so it would be better to not adjust the syntax because of this restriction.

Ok, yes, indeed. Useful also to really overwrite initial speed values (unlike limit_to).

What would be the advantage of this?

that you can avoid the "if": true workaround and be more clear about the intent.

I probably don’t understand the problem yet

The problem is not about the set_to operator but the fact that it contains a “dynamic” encoded value. And we need to ensure the correctness of A* and if the encoded value contains values bigger than 1 (maximum priority) for some edges and we use
set_to: bike$my_encoded_value
then the priority in the graph can be lower than what we estimate with getMinWeight which could lead to suboptimal routes for all A* algos.

I would be ok to enforce a limit_to for all queries with an encoded value in its expression but it would have to be an unconditional limit_to with a static value:

{
 "priority": [{
   "if": true,
   "set_to": "0.5 + bike$priority"
 }, {
   "if": true,
   "limit_to": 1.5
 }]
}

It would be less ugly when the "if": true is omitted as we would also need this for multiply_by and limit_to with expressions.

Another solution would be to log the maximum value of the EncodedValue. Or we use the maximum possible value via getMaxInt or getMaxDouble, but then the performance could be affected if this maximum value is far above the used maximum e.g. if a max speed of 140km/h is required we could have a maximum of the underlying EncodedValue of 256km/h due to the bit range. Or we store an additional max value in every EncodedValue - could also be useful for things like: Max speed can get exceeded due to rounding error · Issue #2322 · graphhopper/graphhopper · GitHub. But the problem is then that we mix application and storage too much (we did this with the default value and this was something we had to get rid of fast).

@ratrun can you have a look into my proposal at: Include priority EncodedValue in CustomWeighting by karussell · Pull Request #2333 · graphhopper/graphhopper · GitHub

It comes without any API change and will "just work"™ for custom profiles. The only downside is that the priority EncodedValue requires 4 instead of 3 bits and due to rounding errors there might be tiny differences between the old and new behaviour. I’m currently investigating the tests that required a change and if the routes still look reasonable. It would be very appreciated if you can have a look and test it out in real world for your cases too.

due to rounding errors there might be tiny differences between the old and new behaviour

My typical testroute for a cycle route rcn now contains a short ordinary highway=residential detour, which it didn’t before. I don’t care. Anyway, I was a bit surprised that the empty custom profile does not take this short detour. Here there is a different example where the custom profile and the empty rule do not give the same results:
http://localhost:8989/maps/?point=48.409947%2C15.63715&point=48.407127%2C15.660453&locale=de&elevation=true&profile=bike&details=surface&details=smoothness&use_miles=false&selected_detail=distance&layer=TF%20Cycle

Here is my custom_bike profile result with an empty rule:

I played around with the following custom rule and different factors:

{
 "priority": [
  {
   "if": "true", 
   "multiply_by": 0.5
  }]
}

I looks as if the higher the factor becomes the more it avoids segments with higher priority. A factor of 0.75 still prefers the Eurovelo 6 and with 0.8 one gets the same result as compared to the first screenshot.

I just tried this route with my proposed PR (branch include_priority) and I cannot see a difference anymore. Can you confirm that this works for you (at least for this case) :slight_smile: ?

Sorry, but it does not give the same results for me. I once again have switched to the include_priority branch, reduced my config.yml into

  graph.flag_encoders: bike
  graph.encoded_values: surface,smoothness,road_class,road_class_link,road_environment,max_speed,road_access,toll,track_type

  profiles:
    - name: bike
      vehicle: bike
      weighting: fastest
      
    - name: custom_bike
      vehicle: bike
      weighting: custom
      custom_model_file: empty

and deleted the graph folder. I’m getting exactly the results as described.

Ah, it is important to use

 profiles:      
    - name: custom_bike
      vehicle: bike
      weighting: custom
      custom_model:
        distance_influence: 0

for a really identical behaviour otherwise you get a similar behaviour to
weighting: short_fastest. As the default distance_influence is 70 for custom weighting.

Thank you, now I’m getting the same results. :smiley:

I still have one more question: What is the best rule to increase the influence of the existing priority? E.g. if I want to go an the “best” priority ways and take longer detours into regard. I would assume that it is now possible to prefer “rcn” cycle relations, but I couldn’t mange this with a custom rule.

This rule does something similar to what I intend:

{
 "priority": [{
   "if": "bike_network == MISSING",
   "multiply_by": 0.5
 }]
}

Good that it works now :slight_smile:

Do you mean you want to increase the importance of the priority stored in bike$priority?

Then this could work (?):

{
 "priority": [{
   "if": "true",
   "multiply_by": 10
 }]
}

I would assume that it is now possible to prefer “rcn” cycle relations, but I couldn’t mange this with a custom rule.

Does something like this work for you?

{
 "priority": [
  {
   "if": "bike_network != REGIONAL",
   "multiply_by": 0.5
  }
 ]
}

Or bike_network == REGIONAL and multiply_by: 2 if you want this for the server-side only

Thank you.

Do you mean you want to increase the importance of the priority stored in bike$priority ?

Yes, this is what I want. I tried your suggestions, but I couldn’t find any effect of the rule

{
 "priority": [{
   "if": "true",
   "multiply_by": 0.5
 }]
}

when the factor is in the allowed range from some small value of 0.0001 to 1.0.
The other one

{
 "priority": [{
   "if": "bike_network != REGIONAL",
   "multiply_by": 0.1
  } ]
}

boosts the not regional cycle routes when the factor is low, but does not have any effect on the regional ones.

and multiply_by: 2 if you want this for the server-side only

Does that mean that a server side rule does not need to follow the client side factor limitation?

Yes. The problem is the landmark algorithm where only a decrease of the edge weights is allowed to avoid incorrect/suboptimal routes. But when we know that the edge weights are bigger already while the LM preparation then a factor bigger 1 is fine.

when the factor is in the allowed range from some small value of 0.0001 to 1.0.

Did you try a factor bigger than 1 for server-side?

Powered by Discourse