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

Fixes some issues with turn cost times. #1586

Merged
merged 6 commits into from Apr 1, 2019

Conversation

@easbar
Copy link
Collaborator

easbar commented Mar 29, 2019

Fixes two of the three issues mentioned in #1585.

Added some tests and did some cleanup in some tests, but the actual fixes are one-liners.

easbar added 5 commits Mar 28, 2019
Signed-off-by: easbar <easbar.mail@posteo.net>
Signed-off-by: easbar <easbar.mail@posteo.net>
…ting#calcMillis.

Signed-off-by: easbar <easbar.mail@posteo.net>
Signed-off-by: easbar <easbar.mail@posteo.net>
minor
Signed-off-by: easbar <easbar.mail@posteo.net>
@@ -83,7 +83,7 @@ public Path extract() {
setFromNode(currEdge.adjNode);
reverseOrder();
currEdge = edgeTo;
int prevEdge = nextEdgeValid ? sptEntry.edge : EdgeIterator.NO_EDGE;
int prevEdge = EdgeIterator.Edge.isValid(sptEntry.edge) ? sptEntry.edge : EdgeIterator.NO_EDGE;

This comment has been minimized.

Copy link
@easbar

easbar Mar 29, 2019

Author Collaborator

This fixes the turn cost time evaluation at the meeting node


return millis + turnCostsInMillis;
return millis + 1000 * turnCostsInSeconds;

This comment has been minimized.

Copy link
@easbar

easbar Mar 29, 2019

Author Collaborator

Here we have to convert the turn costs to milliseconds to make the turn cost time calculation consistent with the weight calculation.

This comment has been minimized.

Copy link
@karussell

karussell Mar 30, 2019

Member

Maybe we should decide before going more public with turn costs to make a breaking change and use the same unit for turn costs? I think I stumbled over the same problem somewhere.

This comment has been minimized.

Copy link
@easbar

easbar Mar 31, 2019

Author Collaborator

You mean store the turn costs as milliseconds ? In this PR or is this just a reminder for later ? Storing turn costs as milliseconds seems a bit extreme, seconds should be enough ? Maybe use 0.1 seconds ?

This comment has been minimized.

Copy link
@karussell

karussell Mar 31, 2019

Member

You mean store the turn costs as milliseconds ?

I would keep storing them as seconds (and later use the same EncodedValue classes to define a different precision), but shouldn't we use the same unit ms in the Java API turnCostEncoder.getTurnCost? Furthermore to make it more clear maybe we should rename calcTurnWeight into calcTurnCost? Or even introduce a separate method calcTurnMillis like we have for normal weighting ... but are there really cases where a priority to avoid a turn is different to the turn time?

In this PR or is this just a reminder for later?

Good question - probably later :)

This comment has been minimized.

Copy link
@easbar

easbar Apr 1, 2019

Author Collaborator

but shouldn't we use the same unit ms in the Java API turnCostEncoder.getTurnCost

The way I understood it so far is that the turn costs do not necessarily have to be turn times, maybe someone is using an advanced weighting and drivers just do not want to take turns so each turn adds some penalty (but not necessarily time) to some advanced weight function ?

Furthermore to make it more clear maybe we should rename calcTurnWeight into calcTurnCost?

Hmm yeah makes sense because we call it turn cost everywhere else, then again calcTurnWeight fits calcWeight in the TurnWeighting class.. no idea.

but are there really cases where a priority to avoid a turn is different to the turn time

Hmm I could imagine this, but don't know if anybody does this :)

This comment has been minimized.

Copy link
@karussell

karussell Apr 1, 2019

Member

Hmm I could imagine this, but don't know if anybody does this :)

But which use case do you have in mind where "time != weight"?

The only thing that comes to my mind is an efficient storage. E.g. only two bits are stored and the 0 maps to no turn time, the 1 maps to 5 seconds, the 2 maps to 10 seconds and the 3 maps to infinity (turn restriction). But once we would introduce something like the MappedDecimalEncodedValue I do not see a reason to use the same ms as unit.

Maybe there are already users of this more generic turn cost feature, but if we document this change this is no problem IMO. And we should not hesitate to make this API more consistent before it is used more widespread.

BTW: As the defaultUTurnCost is in seconds - is this correct?

This comment has been minimized.

Copy link
@easbar

easbar Apr 1, 2019

Author Collaborator

see #1590

TurnWeighting turnWeighting = new TurnWeighting(new ShortestWeighting(encoder), turnCostExt);
// todo: for the shortest weighting turn costs cannot be interpreted as seconds ? at least when they are added
// to the weight ? how much should they contribute ?
// assertEquals(105, turnWeighting.calcWeight(edge, false, 0), 1.e-6);

This comment has been minimized.

Copy link
@easbar

easbar Mar 29, 2019

Author Collaborator

Any thoughts here ?

This comment has been minimized.

Copy link
@karussell

karussell Mar 30, 2019

Member

What do you think about deprecating shortest weighting in general? It does not make sense in practise. Most people say they need "shortest" for car (hear this often for taxis as they calculate the cost based on the distance), but what they really mean is a "fastest" cost calculation with a tight distance weight.

This comment has been minimized.

Copy link
@easbar

easbar Mar 31, 2019

Author Collaborator

Ok I see. But not sure if it should be really deprecated, because someone might find it still useful. We could emphasize somewhere that the ShortestFastestWeighting is typically what most people are looking for. Anyway the question how turn costs should contribute to weightings that are not purely based on time remains also for the ShortestFastestWeighting. In ShortestFastestWeighting this relation is controlled by the time/distanceFactors.

This comment has been minimized.

Copy link
@karussell

karussell Mar 31, 2019

Member

For now IMO the turn costs for ShortestWeighting must be 0 as there are no ways to introduce a timely slow down or priority stuff.

In ShortestFastestWeighting the turn cost adds to the time (and gets multiplied by the timeFactor)?

This comment has been minimized.

Copy link
@easbar

easbar Apr 1, 2019

Author Collaborator

Yes, I agree as long as we do not allow giving less priority to routes with many turns etc. the shortest weighting should pay attention to turn restrictions, but the turn costs (assuming they are turn times) should not be included. However to make this work we would in fact have to do this:

Or even introduce a separate method calcTurnMillis like we have for normal weighting ... but are there really cases where a priority to avoid a turn is different to the turn time?

because right now the TurnWeighting simply adds the turn costs without knowing what it's underlying Weighting is.

This comment has been minimized.

Copy link
@karussell

karussell Apr 1, 2019

Member

Hmmh, indeed. This is ugly and one reason more (for me) to remove ShortestWeighting :D

This comment has been minimized.

Copy link
@easbar

easbar Apr 1, 2019

Author Collaborator

see #1590

Copy link
Member

karussell left a comment

Thanks for this PR - impressive how small the actual changes are 👍

@karussell karussell added the bug label Mar 30, 2019
@karussell karussell added this to the 0.13 milestone Mar 30, 2019
@easbar

This comment has been minimized.

Copy link
Collaborator Author

easbar commented Apr 1, 2019

Will merge this and open separate issue for the turn cost issues discussed here.

@easbar easbar merged commit ff5590c into master Apr 1, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@karussell karussell deleted the time_problems branch Apr 1, 2019
@easbar easbar referenced this pull request Apr 1, 2019
4 of 4 tasks complete
stevensnoeijen added a commit to stevensnoeijen/graphhopper that referenced this pull request Apr 2, 2019
* master: (32 commits)
  Fixes some issues with turn cost times. (graphhopper#1586)
  Android: VTM 0.11.0 OpenGL vector map library (graphhopper#1587)
  Cannot set a blocked edge to accessible again (graphhopper#1541)
  testing against JDK 12+13 instead 11+12
  web: updated tracker for maps
  Fixes compiler warning.
  Fixes wrong routing occurring when stall on demand is used with virtual via-nodes. (graphhopper#1581)
  Update README.md
  Update README.md
  Fix broken link (graphhopper#1578)
  i18n: updated zh_CN
  web ui: updated a few dependencies
  Use Leaflet.Heightgraph to show PathDetails (graphhopper#1569)
  updated android-maven-plugin
  fix latest stable links in readme
  Fix typo (graphhopper#1575)
  minor fix to parent version in android ap
  avoid traversalKey and use traversalId or edgeKey instead, depending on the use case graphhopper#1549
  Fixes endless recursion in AllCHEdgeIterator.
  braking change: rename to always use traversal key, fixes graphhopper#1549
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.