Feature maximum visited nodes #681
Conversation
|
@karussell I think we have an issue with our current travis configuration. I forgot to run the whole test-suite before pushing and it failed for me locally. Since the js tests succeed, travis seems to think that all tests succeed. Btw, I am currently fixing the issues ;). |
|
Ok. PR should be fine. What do you think? |
|
|
||
| protected boolean isMaxVisitedNodesExceeded() | ||
| { | ||
| return maxVisitedNodes > getVisitedNodes(); |
There was a problem hiding this comment.
but not important to add a test as we have your nice PR soon and throw this away :)
There was a problem hiding this comment.
Ah yes, we can actually delete the isMaxVisitedNodesExceeded method, since it is not used here. We delegate the maxVisitedNodes to the Dijkstra and the Dijkstra cares about the maxVisistedNodes.
|
Thanks - looks really good!
Do you mean that although tests are failing locally, travis is fine with it because only the JS tests are passing? |
| @@ -1093,10 +1096,15 @@ public GHResponse route( GHRequest request ) | |||
| QueryResult fromQResult = qResults.get(0); | |||
|
|
|||
| double weightLimit = request.getHints().getDouble("defaultWeightLimit", defaultWeightLimit); | |||
| int maxVisistedNodesForRequest = request.getHints().getInt("routing.max_visited_nodes", maxVisitedNodes); | |||
There was a problem hiding this comment.
Here the parameter shouldn't be plain "max_visited_nodes" ?
There was a problem hiding this comment.
problem is that we are very inconsistent with this and it has a reason: from URL or JS one expects some_thing from within Java one expects someThing. What we could do is internally convert all someThing into some_thing or migrate all remaining someThing to my preferred naming some_thing "somehow"
There was a problem hiding this comment.
Yes we can change the name in both cases (config and url) so we do not have the routing. in front. I thought it would be better understandable since the number of parameters is growing.
There was a problem hiding this comment.
The routing in front is good as indeed the count is growing. Also please for now use someThing notation - I've created #682 to make everything more consistent or to be able to use both.
There was a problem hiding this comment.
Ok, so I shall use camel case in the url, as well?
Yes, have a look at this test. It fails but travis does not fail https://travis-ci.org/graphhopper/graphhopper/jobs/114835803#L1152 |
Outch - ugly! Thanks for finding this! |
|
Ok, I updated my branch. I renamed the URL parameter using camelCase. |
|
Thanks a lot! One minor in the exception -> routing.max_visited_nodes. Would you also squash the commits into one? |
boldtrn
force-pushed
the
feature_maximum_visited_nodes
branch
from
a90fe6a
to
7046e05
Compare
Fixed some formatting issues Added security catch if URL parameter contains values greater than the configured Fixed failing Tests Updated according to comments Updated according to comments Update
boldtrn
force-pushed
the
feature_maximum_visited_nodes
branch
from
7046e05
to
c5b6b7f
Compare
|
Squashed it |
Limit search via maximum visited nodes. Fixes #473
|
Thanks - merged! |
This PR contains the ability to limit the number of visitedNodes that a non-ch RoutingAlgorithm visits.
This can be specified in the config and via an URL parameter.
Fixes #473 and therefor #104