Use regular weighting if no CH weighting is available

[boldtrn]

This PR allows to use weightings that are not prepared with CH to be used instead of throwing an Exception.

A possible use-case could be a routing that considers live-traffic, but in general most routes are calculated without considering live-traffic (this would be CH weighting).

In general, I think falling back to a regular weighting is better than throwing an exception (at least in most cases).

This fixes #491

[boldtrn]

I just had this issue, after rerunning the tests, this was solved. Has been also reported in #625

npm ERR! Error: Attempt to unlock leaflet@^0.7.7, which hasn't been locked
npm ERR!     at unlock (/home/travis/.nvm/v0.10.36/lib/node_modules/npm/lib/utils/locker.js:44:11)
npm ERR!     at cb (/home/travis/.nvm/v0.10.36/lib/node_modules/npm/lib/cache/add-named.js:32:5)
npm ERR!     at /home/travis/.nvm/v0.10.36/lib/node_modules/npm/lib/cache/add-named.js:41:20
npm ERR!     at /home/travis/.nvm/v0.10.36/lib/node_modules/npm/lib/utils/locker.js:30:7
npm ERR!     at cb (/home/travis/.nvm/v0.10.36/lib/node_modules/npm/node_modules/lockfile/lockfile.js:149:38)
npm ERR!     at /home/travis/.nvm/v0.10.36/lib/node_modules/npm/node_modules/lockfile/lockfile.js:177:38
npm ERR!     at Object.oncomplete (fs.js:108:15)
npm ERR! If you need help, you may report this *entire* log,
npm ERR! including the npm and node versions, at:
npm ERR!     <http://github.com/npm/npm/issues>

[karussell]

This fixes #491 ? Nice!

We should allow this only after an explicit config setting change as the performance difference and RAM usage etc could be dramatic and needs to be known up-front.

Regarding the error. See #632

[boldtrn]

Makes Sense. I updated this PR. (And created a Merge Conflict with my latest PR ...). Should fix #491, I guess.

[karussell]

Thanks!

The control flow is via try-catch and more implicit. I would avoid this and specify the ch.flexibleMode parameter for every query:

String perRequestCHFlexibleMode = request.getHints().get("ch.flexible_mode", false);
if(perRequestCHFlexibleMode && !chFlexibleMode) {
   rsp.addError(new some exception);
   return Collections.emptyList();
}

if (chEnabled && !perRequestCHFlexibleMode) ...

Which means the slow requests are explicitely wanted not only from the server side but also expected from the client side.

Also we need test for this and the exception ... I can add those.

BTW: in JSON and URL we do not use camel case but in java params we do like e.g. defaultWeightLimit ... maybe we should convert all camelcase parameters to underscore... Of course, this is for another issue.

[boldtrn]

@karussell I updated my branch. I am not sure if we really should add the flexible mode to the request as well. This is an overhead on the client-side. Clients have to figure if the should query the flexible mode or the speed mode.

[karussell]

Thanks!

This is an overhead on the client-side. Clients have to figure if the should query the flexible mode or the speed mode.

Yes, sure. But with this the client decides e.g. if he wants more speed or more features (roughly speaking). How should we handle such a logic on the server side?

[boldtrn]

Well, we could query if there is CHWeighting that matches, if not just use a NON-CHWeighting. In this way we could work around the try catch exception stuff.

On the other hand, this might slow down requests out of nothing (from a user perspective). So I am not sure which solution is preferred.

[karussell]

In this way we could work around the try catch exception stuff.

I mean there are currently also cases like turn restrictions and heading which are not supported by CH where the client should be able to force flexibility mode even if the weighting supports CH.

On the other hand, this might slow down requests out of nothing (from a user perspective)

Exactly that is the reason I think that the overhead on the client side is okay, as the client then knows it needs to handle larger response times and potentially notify the user.

[boldtrn]

Yes probably you are right.

[boldtrn]

I mean there are currently also cases like turn restrictions and heading which are not supported by CH where the client should be able to force flexibility mode even if the weighting supports CH.

I am currently looking into this. I found that if we use a weighting that was prepared for ch and query the flexible mode (e.g. for TurnRestrictiions) we get an exception since the graph is expecting CHEdges. Any ideas what to do in this case? Should we ignore that case for now? Here is the StackTrace:

ERROR com.graphhopper.http.GHErrorHandler - com.graphhopper.storage.BaseGraph$EdgeIterable cannot be cast to com.graphhopper.util.CHEdgeIteratorState, via:http://localhost:8989/route
java.lang.ClassCastException: com.graphhopper.storage.BaseGraph$EdgeIterable cannot be cast to com.graphhopper.util.CHEdgeIteratorState
    at com.graphhopper.routing.util.LevelEdgeFilter.accept(LevelEdgeFilter.java:50)
    at com.graphhopper.routing.AbstractRoutingAlgorithm.accept(AbstractRoutingAlgorithm.java:78)
    at com.graphhopper.routing.DijkstraBidirectionRef.fillEdges(DijkstraBidirectionRef.java:199)
    at com.graphhopper.routing.DijkstraBidirectionRef.fillEdgesFrom(DijkstraBidirectionRef.java:157)
    at com.graphhopper.routing.AbstractBidirAlgo.runAlgo(AbstractBidirAlgo.java:72)
    at com.graphhopper.routing.AbstractBidirAlgo.calcPath(AbstractBidirAlgo.java:63)
    at com.graphhopper.GraphHopper.getPaths(GraphHopper.java:1128)

[karussell]

Do you mean if we set car|fastest and ask for car|fastest but flexible mode we get this exception? That should not be the case and the base graph should be used. Hmmh, and the LevelEdgeFilter indicates that still the PrepareCH factory was used to create the algo.

[boldtrn]

@karussell exactly. Thanks for the hint. There is already a CHAlgorithmFactory registered for that weighting. We could create a new tempFactory for that case. We could check chEnabled && perRequestCHFlexibleMode && request.getWeighting.equals(getChWeighting()) then we would create a new simplefactory. What do you think?

[karussell]

Ah, thanks - this is indeed very ugly. Maybe instead of replacing we should chain the factories? E.g. if no matching algorithm is found in CH factory (flexibility=true could avoid matching) it is forwarded to the simple factory ... hmmh, I need to think about this a bit as there will come some more factories soonish :)

[boldtrn]

@karussell What do you mean by replacing and chaining? Maybe we could add a property to the AbstractWeighting that defines if the weighting is CH or not. We could change the equals method of the AbstractWeighting to also match that property. Therefore, we could have different Factories for CH and Non-Ch. What do you think?

[karussell]

With chaining I mean that PrepareContractionHierarchies would extend RoutingAlgorithmFactorySimple and call super.createAlgo if there is no matching algorithm.

E.g.

class PrepareContractionHierarchies {
...
  public RoutingAlgorithm createAlgo( Graph graph, AlgorithmOptions opts ){
      if(opts.getHints().get("flexibility", false)) {
         super.createAlgo(graph, opts);
      } else {
          /* do CH stuff */
      }
  }
}

Or instead the map we should have a list and create a more sophisticated "parameter matching" selecting the appropriate factory/algorithm.

Maybe we could add a property to the AbstractWeighting that defines if the weighting is CH or not

There is and should be no difference between a CH and a flexible weighting :/

[boldtrn]

With chaining I mean that PrepareContractionHierarchies would extend RoutingAlgorithmFactorySimple and call super.createAlgo if there is no matching algorithm.

Ah ok, now I get your idea. The benefit would be that we do not have to change the GraphHopper class

Or instead the map we should have a list and create a more sophisticated "parameter matching" selecting the appropriate factory/algorithm.

Also possible, but might become to complex if there will be a lot of new factories? Not sure.

There is and should be no difference between a CH and a flexible weighting :/

Yes you are right. But then the mapping from weighting to algoFactory is not correct? Maybe we should be map the combination of weighting and ch to one factory. Or extend the factory.

[karussell]

Can you please review my changes here?

I've introduced a new decorator interface making the request to factory matching more customizable, interesting for future additions of new algorithms and every factory can 'enhance' or even replace the currently used factory. The costs are that all is now a bit more complex.

And although the Weighting.match method is now 'cleaner' (takes just one parameter) it does the check also against the strings and not the objects. Not sure if this is an improvement :).

This takes me to another issue. The point where we need 'strings' (fastest/shortest/car/...) and where 'real objects' is very tricky, as e.g. we need real Weighting objects for the CH version of the GraphHopperStorage already BUT we cannot use PrepareContractionHierarchies.getWeighting as PrepareContractionHierarchies needs the GraphHopperStorage. This lead me to the decision in the decorator to decouple the Weighting- and the PrepareContractionHierarchies-objects.

The graphHopper.getWeightingForCH method is now also moved into the new CHAlgoFactoryDecorator

I made sure that the case where you got an exception now works via testFlexMode_631

As we need no preparation for measurement now I've introduced this setting in the Measurement class, which partly fixes #492 now already.

[karussell]

This works for some other stuff I'm currently working on but the decorator stuff is then a bit duplicated. But this improvement can be done in another issue once we then really have multiple decorators :)

update: I would like to merge if someone else could have a quick look at it :)

[boldtrn]

Sorry for the delayed review. I like the decorator approach.

I've introduced a new decorator interface making the request to factory matching more customizable, interesting for future additions of new algorithms and every factory can 'enhance' or even replace the currently used factory. The costs are that all is now a bit more complex.

But there was no changed behavior here, right?

Not sure if this is an improvement :)

Not sure as well ;)

This takes me to another issue. The point where we need 'strings' (fastest/shortest/car/...) and where 'real objects' is very tricky, as e.g. we need real Weighting objects for the CH version of the GraphHopperStorage already BUT we cannot use PrepareContractionHierarchies.getWeighting as PrepareContractionHierarchies needs the GraphHopperStorage. This lead me to the decision in the decorator to decouple the Weighting- and the PrepareContractionHierarchies-objects.

Maybe we could wrap it? Returning PrepareContractionHierarchies.getWeighting if it's available, else return the local Weighting in the Decorator .

request.setWeighting(getCHWeightings().isEmpty() ? "fastest" : getCHWeightings().get(0).toString());
I would prefer to extract the getDefaultWeighting part of this line. Or wrap it in two lines. This would improve the readability a lot.

decorator.decorate
I would prefer a different naming of the decorate method. Something like getDecoratedAlgorithmFactory or something like that would be better in my opinion.

[karussell]

Maybe we could wrap it? Returning PrepareContractionHierarchies.getWeighting if it's available, else return the local Weighting in the Decorator

Not sure how you meant this. There are two things that I do not like much:

1. the duplication of the weightings, first is the chWeightingList as string and second we have the weighting list as objects in the decorator.
2. the weighting list in the decorator is decoupled from the preparation list in the decorator, but this is important. Because we need the weighting list to create the graph storage but to create the preparations we need the graph storage.

I would prefer to extract the getDefaultWeighting part of this line. Or wrap it in two lines. This would improve the readability a lot.

Hmmh, this way we would have yet another method in the GraphHopper class. Maybe we can move the "chWeighting<string>" stuff into the decorator as well?

I would prefer a different naming of the decorate method. Something like

okay :)

[boldtrn]

the weighting list in the decorator is decoupled from the preparation list in the decorator, but this is important. Because we need the weighting list to create the graph storage but to create the preparations we need the graph storage.

Yes, I had an idea about unifying both Weightings, by creating a magic getWeighting method, that checks the current state of the decorator and returns the appropriate weighting. But I think this involves to much magic.

Hmmh, this way we would have yet another method in the GraphHopper class. Maybe we can move the "chWeighting" stuff into the decorator as well?

Would be nice, but would this work? I mean we need to know the weighting of the request before getting the decorator, right? Maybe we should set this as fallback in the Request/HintsMap?

[karussell]

Would be nice, but would this work? I mean we need to know the
weighting of the request before getting the decorator, right?

See the updated branch here, where I moved more CH related stuff into the decorator, also the thread pool stuff which really did not belong to the GraphHopper facade :). There is also chFactoryDecorator.getDefaultWeighting() :)

Now the decorator serves no longer one purpose only - this might be separated later:

1. config & init of CH (weighting related)
2. CH preparation
3. routing algo factory decorator

If there are no objections I'll merge this in the next days.

[karussell]

Ah, forgot to rename the method decorate. Maybe getDecoratedRAF ;) ?

Also picking the default weighting from the CH decorator even if this is disabled is probably not that ideal (but was identical before):

if (request.getWeighting().isEmpty())
request.setWeighting(chFactoryDecorator.getDefaultWeighting());

[karussell]

Ok, renamed and moved further minor stuff into the CH related decorator

[boldtrn]

@karussell Nice! Now the Decorator becomes pretty crowded and reliefs GraphHopper.java.

getDecoratedRAF

Thanks for choosing getDecoratedAlgorithmFactory, that's the better name in my opinion. It took me quite some time to remember what RAF means 👍.

In general I like it. Probably we should merge this ASAP. What do you think?

[karussell]

Nice!

Good that you like it :) !

Now the Decorator becomes pretty crowded and reliefs GraphHopper.java.

Yes, but the decorator can be splitted later and is really focussed on the CH part. Making it later easy to add others as well.

Probably we should merge this ASAP.

Yes, will do today (or early tomorrow)!

[karussell]

Thanks again - merged!