Fix bug in non-CH AlternativeRoute #2603

karussell · 2022-06-13T10:23:53Z

Should fix #2566

Using the fixes of this PR the plateaus show up properly now:

See the alt_route_debug branch for the necessary changes in MiniGraphUI to see this (using berlin data). Note 1. the two red lines are the best and alternative route in the end, 2. same colors does not mean the plateaus are related and 3. the two block circles are the start and end locations.

Before it was a mess and showed hundreds of potential candidates (many of them were overlapping) that we had to loop through.

The speed improvement should also show up for node-based.

This reverts commit fb39266.

karussell · 2022-06-13T10:24:33Z

core/src/main/java/com/graphhopper/routing/AlternativeRoute.java

+                        SPTEntry correspondingToEntry = bestWeightMapTo.get(nextToTraversalId);
+                        if (correspondingToEntry != null) {
+                            if (traversalMode.isEdgeBased())
+                                correspondingToEntry = correspondingToEntry.parent;
+                            if (correspondingToEntry.edge == fromSPTEntry.edge)
+                                return true;
+                        }
                    }


Here we rejected too many possibilities.

karussell · 2022-06-13T10:25:08Z

core/src/main/java/com/graphhopper/routing/AlternativeRoute.java

+                        SPTEntry otherFromEntry = bestWeightMapFrom.get(nextFromTraversalId);
                        // end of a plateau
-                        if (nextFromSPTEntry == null)
-                            break;
-
-                        // is the next from-SPTEntry on the plateau?
-                        if (prevToSPTEntry.edge != nextFromSPTEntry.edge)
+                        if (otherFromEntry == null ||
+                                otherFromEntry.parent != prevFrom ||
+                                otherFromEntry.edge != prevToSPTEntry.edge)


And here we rejected too few possibilities as we didn't properly stop the search when the plateau ends.

karussell · 2022-06-13T10:26:34Z

core/src/main/java/com/graphhopper/routing/AlternativeRoute.java

@@ -165,7 +163,7 @@ public void setMaxPaths(int maxPaths) {
     */
    public List<AlternativeInfo> calcAlternatives(int from, int to) {
        AlternativeBidirSearch altBidirDijktra = new AlternativeBidirSearch(
-                graph, weighting, traversalMode, maxExplorationFactor * 2);
+                graph, weighting, traversalMode, maxExplorationFactor);


This factor here is very confusing so I removed this but this required a few minor changes like above 0.8 -> 1.6 etc

easbar · 2022-06-13T10:53:26Z

Thanks, very nice! I tried this for an example request I used previously (profile=car&point=49.91,11.31&point=47.83,11.53) and the query time went down from 1min52s to 15s. So it definitely went from almost unusable to usable :) For node-based it did not change so much: around 4s in both cases, but I did not measure this thoroughly. Briefly looking at a few examples using maps2 the suggested alternative routes also seem plausible 👍

tools/src/main/java/com/graphhopper/tools/Measurement.java

…2603

karussell · 2022-06-16T14:40:52Z

The change regarding the exploration factor was done here: ba19695

IMO the exploration factor could be reduced further without compromising quality or alternative_rate, but the algorithm needs to be adjusted and much smaller but "promising" plateaus (e.g. still a certain limit) would have to trigger the further exploration and the exploration goes along a branch (of the forward shortest path tree) only until it stays on the plateau.

Currently likely the problem with a too small exploration factor is that the plateau size will be too small even for good alternatives.

* fix travis release for tagged commits; use jdk 17 instead of 11 for github releases * allow skipping the LM constraint check for A* (graphhopper#2544) * LM constraint should be ignored for A* * make it possible to skip checkLMConstraints call * move checkLMConstraints call to LMSolver. Fix createSolver for mixed preparations * avoid fundamental default change but throw proper exception when lm.disable parameter missing * Add names for travis build jobs (to be shown under 'Jobs and Stages on GitHub)' * Disable doclint explicitly * travis: only keep release build, related to graphhopper#2552 * Clean up javadoc comments (graphhopper#2569) * fix javadoc errors * remove javadoc plugin * make comment more readable * include javadoc again, revert unreadable comments * disable doclint for javadoc * Add flex alternatives to measurement * Do not remove SPT elements from the priority queue, speeds up flexible algorithms (graphhopper#2571) * Split flag encoders into encoded values and tag parsers (graphhopper#2561) * Remove final from VehicleEncodedValues * Log available country rules (graphhopper#2579) Co-authored-by: Thomas Butz <thomas.butz@optitool.de> * Give access to vehicle tag parsers * Update bestFwd/BwdEntry for flexible algorithms when deleting SPT entries, fix graphhopper#2581 * Travis: Fix warnings in build config validation It said: Build config validation root: deprecated key sudo (The key `sudo` has no effect anymore.) root: missing os, using the default linux root: key matrix is an alias for jobs, using jobs * Add VehicleEncodedValues#isHGV * Use more recent node/npm versions to fix the build (graphhopper#2583) * Update readme for 5.3 * update deployment guide (graphhopper#2576) * update deployment guide * Update deploy.md * review comments * mention max_visited_nodes * Replace deprecated dropwizard parameter types (graphhopper#2574) Co-authored-by: Thomas Butz <thomas.butz@optitool.de> * Use epsilon=0.9 for LM map-matching * Update changelog * Use edge keys consistently, store keys in CH shortcuts (graphhopper#2567) * Add comment about slow non-CH alternatives * Remove outdated/wrong comments * Add a mapmatching test * Remove 'shared' encoded values with $ sign in name (graphhopper#2585) * remove OSMAccessParser * removed $ from encoded values * no if clause necessary * fix changelog * downgrade todonow * Matrix client improvements (graphhopper#2587) * tmp * fix tests * update okhttp to 4.9.3 * fixes regarding code review Co-authored-by: easbar <easbar.mail@posteo.net> * European toll fallback rules (graphhopper#2450) Co-authored-by: Thomas Butz <thomas.butz@optitool.de> * use for loop in PathDetailsBuilderFactory as discussed in graphhopper#2585 (graphhopper#2589) * Fix: Put back block_private and block_fords for racingbike, this got lost in graphhopper#2561 * Make motor vehicle and hgv properties configurable for roads encoder (graphhopper#2594) * Disable edge+alt measurements, they take too long * Use separate deleted flag for SPTEntry, prevent exception when calculating routes with non-feasible approximator (graphhopper#2600) Also revert epsilon=0.9 'fix' in map matching now that we know what is going on * LMApproximator: Align variable names with equation numbers * Log map-matching took value in ms instead of seconds (graphhopper#2591) Co-authored-by: Andi <easbar.mail@posteo.net> * rename StringIndex to EdgeKVStorage in preparation for graphhopper#2597 * windows does not need a separate documentation due to WSL (graphhopper#2599) * windows does not need a separate documentation due to WSL * link to install steps directly * EdgeKVStorage: store more than Strings (graphhopper#2597) * copied code and tests from byteindex branch * a bit more docu and tests * behaviour change: throw exception if string is too long and so cut string before storing way_name * renamed StringIndex -> EdgeKVStorage * more consistent: reject null values for all cases * include lost changes again * ensure same version of storage is used * changes from review * fix variable names for key handling * comments to javadoc * better 'compression' in case of identical map with byte[] array * throw exception earlier * Fix bug in non-CH AlternativeRoute (graphhopper#2603) * fix plateau-end check * Revert "Disable edge+alt measurements, they take too long" This reverts commit fb39266. * increase count to 10 for Measurement * AlternativeRoute: follow up refactoring, graphhopper#2603 * MiniGraphUI fixes to make it work again * move quickstart info into more prominent installation of README (graphhopper#2605) * Measurement: reduce routingCH_alt after it was accidentally raised in graphhopper#2603 * Remove GraphHopperStorage (graphhopper#2606) * Rename: GraphHopper#getGraphHopperStorage -> GraphHopper#getBaseGraph * Remove GraphHopperStorage * reduce benchmark load e.g. exclude too slow LM16 case * AlternativeRoute: closer to CH version; default is now a better compromise of speed vs. alternatives found (alternative rate is 1.8 for DE and 1.9 for bavaria) * Move/Rename: EncodingManager.Access -> WayAccess * AlternativeRoute: fix test * Clean up EncodingManager Builder * minor: Use Bike/FootNetwork.KEY * Rename freshFlags -> refreshFlags * BaseGraph: remove copying flags on write, see graphhopper#2593 * added elevation data to reduce internet-dependence while running test * Add comment about GraphHopper#init * Merged graphhopper#2593, i.e. revert b8d9b55 Co-authored-by: Andi <contactammmagamma@gmail.com> Co-authored-by: Thomas Butz <thomas.butz@optitool.de> * Add another elevation file used in tests * Use access and speed EVs rather than flag encoder for GHUtility#setSpeed * Fix broken imports * More use access and speed EVs instead of encoder for GHUtility#setSpeed * Replace flag encoder with turn cost encoded value in turn cost provider * Remove FlagEncoder from ShortestWeighting and CustomWeighting * Find strongly-connected components of station (stop) graph (graphhopper#2608) * Load EncodingManager from properties instead of config (graphhopper#2607) * Helper.cutString should be only for KVStorage and reduce length to 250 to fix graphhopper#2609 * CHPreparationGraph bug fix: unsigned shift required (and increase limit for getKeyWithFlags graphhopper#2567) * make error message more clear * CHPreparationGraph: revert to previously correct signed shift * Increase maximum edge-based CH key for preparation to 2^30 again (graphhopper#2612) * Add test for large edge ID, graphhopper#2612 * custom_model: introduce value expression (graphhopper#2568) * value expression: findMax needs to be evaluated * less strict if * or + operator * refactored to findMinMax * made core tests pass * fix some more tests * fix more tests * add encoded values to initialization plus tests * check minimum for custom_model of query * improve on check of minimum and negative factors * less overhead for constant values * use Double.parse instead of ExpressionEvaluator if number to significantly improve speed * minor cosmetics * clarify LATER and removed TODO NOW * adapted docs * minor optimization and cleanup * fix FindMinMax.checkLMConstraints call * do not allow / as operation * reduce power and complexity of right hand side a bit more and allow only a single EncodedValue for now * better readable via extra MinMax class instead of double[] * exclude changes from master * a bit more comments * Create weighting after graph in two tests * Update custom model editor: operator values are strings now * Update custom model editor: validation + auto-complete for rhs expressions (graphhopper#2615) * Allow graph.encoded_values for which there is no tag parser * Use actual speed maximum rather than flagEncoder#getMaxSpeed() for weighting.getMinWeight() (graphhopper#2614) * Rename getMin/MaxInt -> getMin/MaxStorableInt, getMaxSetValueOrMax -> getMaxOrMaxStorable (graphhopper#2616) * Remove FlagEncoder#getMaxSpeed() (graphhopper#2619) * Remove car/bike_access evs from DefaultEncodedValueFactory, was added only for graphhopper#2523 * CustomModelParser: no need for max speed * EdgeKVStorage: store name and ref tags separately (graphhopper#2598) * copied code and tests from byteindex branch * a bit more docu and tests * behaviour change: throw exception if string is too long and so cut string before storing way_name * renamed StringIndex -> EdgeKVStorage * store name and ref tags separately; create separate path detail for ref; use ref as fallback in instruction text * added text to changelog * minor comment changes * review: fix comments * review comments and add getValue method to avoid creating HashMaps * try if speed up was really from the change getKeyValues->edge.getValue * Revert "try if speed up was really from the change getKeyValues->edge.getValue" This reverts commit 58a34d6. Co-authored-by: easbar <easbar.mail@posteo.net> * update i18n * Remove FlagEncoder (graphhopper#2611) * EdgeKVStorage: remove smallCache as too much complexity. we accept disk usage increases by 12% and more uniq keys later possible * Move outdoor vehicles list * Relax speed EV requirement for EncodingManager#getVehicles * Check vehicle instead of access/speed in checkProfilesConsistency * reduce distance when average_speed path details can be missing, fixes graphhopper#2620 * Valid if PathDetail is null (graphhopper#2618) * Valid if object is null * Adding my name in contributors * Adjusting indentation * Update CONTRIBUTORS.md Co-authored-by: Rafael Telles <rafael@bzion.com.br> Co-authored-by: Peter <graphhopper@gmx.de> * Support for forward & backward key value pairs (graphhopper#2622) * fix test * minor update to docs for map matching * Update README.md * add more info to exception * profiles.md: updated rhs to strings * minor test change to avoid downloading elevation tile * CustomModelParser: minor cleanup * EncodedValue: bug fix for negateReverseDirection * ConditionalExpressionVisitor: allow '-' as unary operation * fixed link to CGIAR attribution * make global time&distance values consistent with path details and instructions (graphhopper#2626) * make global time&distance values consistent with time&distance path details and also the sum of time&distance of the instructions * minor simplification * mention change in changelog * minor comment fix for graphhopper#2626 * Update github actions v2->v3 * Include destination in instructions (graphhopper#2624) * support for destination and add test for non repetitive instructions when destination is included * include destination:ref * add missing osm file * try getKeyValues instead getValue * try speed of getKeyValues (fetching REVERSE_STATE necessary as getKeyValues is align with 'internal' storage direction) * use simpler edge.getValue call again * Make profile resolving more flexible for web API (graphhopper#2629) * ProfileResolver only resolves name of the profile (graphhopper#2629) * car: do not pass jersey_barrier https://www.openstreetmap.org/node/8857224952 * move cut string method to EdgeKVStorage * EdgeKVStorage: no need for cacheSize parameter anymore * Examples: remove incorrect comment * fix PathDetails e.g. -1 as default for DecimalDetails doesn't work as a value of the EV could be negative too * renamed DouglasPeucker to RamerDouglasPeucker * client-hc: add examples to get translated ("raw") turn instructions * HeightTile: minor correction, DecimalEncodedValueImpl: minor clarification * EdgeKVStorage: allow 32 bits (graphhopper#2632) * allow 32 bits instead of just 31 for EdgeKVStorage * remove incorrect exception * minor change to wayGeo limit check * Maps: Hide gpx export button when custom model box is open, fix graphhopper#2635 * Add hint about custom_model_file vs. custom_model to exception error message (graphhopper#2638) * Add comma in routing logs * Move code that creates EncodingManager from properties to EncodingManager * Return boolean result from BaseGraph#loadExisting * Move code that puts EncodingManager into properties to EncodingManager * add hgt provider * AbstractTiffElevationProvider: use correct long cast * pt: hint on how to use multiple files * Add BaseGraph#debugPrint * new edge smoothing: "ramer" (graphhopper#2634) * initial version of new edge smoothing * improve comments * let's switch the order: first do sampling, then smoothing * use moving_average as name and a few more comments * use same max_elevation like in example config * fix moving_average * again moving_average rename * New average_slope EV (graphhopper#2645) * initial version of new edge smoothing * improve comments * let's switch the order: first do sampling, then smoothing * encoded values for elevation-aware routing: average_slope and max_slope * for tunnels and bridges ignore elevation changes for pillar nodes * SlopeSetter -> SlopeCalculator * fix link for GTFS demo file for graphhopper#2639 * removed defaultIsInfinity and fix bug in DecimalEncodedValueImpl (graphhopper#2646) * removed defaultIsInfinity and fix bug in DecimalEncodedValueImpl * changelog: fix issue number * for useMaximumAsInfinity: also throw an exception if value is too big, but not for infinity * fix merge of master * Revert "for useMaximumAsInfinity: also throw an exception if value is too big, but not for infinity" This reverts commit e3d8826. * print warning if max_width, max_weight etc values are too large * fix link for GTFS demo file for graphhopper#2639 * Revert "fix link for GTFS demo file for graphhopper#2639" This reverts commit d9bb4b9. * NOTICE: updated dependencies and copyright * make ramer edge smoothing more robust, graphhopper#2634 * TurnCostParser: no need for method createTurnCostEncodedValues * improvements for bike (graphhopper#2631) * lower speed for bad smoothness and steps; avoid hazmat; faster if paving_stones * clean up of smoothness handling * revert to 4kmh pushing section speed and tweak speed for certain surfaces * mtb+racing: do not overwrite highway and surface speed if identical in subclass * consider multiple from members for no_entry (graphhopper#2648) * allow configuring turn costs and TransportationMode for RoadsTagParser, related to graphhopper#2460 * add hgv enum to make truck access configurable * add test for hgv encoded value * for now do not log as too many warnings, graphhopper#2646 * SlopeCalculator: skip calculation if PointList is empty or 2D only * Do not get elevations for nodes we don't need while parsing OSM * Custom weighting calcEdgeMillis truncate instead of round * just for consistency and easier migration with/from FastestWeighting * CustomWeighting: handle special case for barrier edges with distance==0 and average_speed>0 * Maps: Add slope detail to elevation diagram (graphhopper#2654) * Custom and fastest weighting calcEdgeMillis round instead of truncate * otherwise they are not always the same, because one multiplies by 3600 and the other by 3.6*1000 ... * Fix pt tests * Disable flaky test * custom and fastest calcEdgeMillis again: make them actually the same * Validate that osm ids are not negative (graphhopper#2652) * Add comment about negative osm ids, code formatting * OSMParsers: don't accept null TagParser * replace car4wd with roads and a custom_model (graphhopper#2651) * Revert "OSMParsers: don't accept null TagParser" This reverts commit 8f5c7b2. * Add urban density encoded value to identify built-up areas (graphhopper#2637) * Rename Development -> UrbanDensity (graphhopper#2637) * Support ";" access delimiter for car access. (graphhopper#2655) * Support ";" access delimiter for car access. One example is motor_vehicle = agricultural;forestry * Add support for ";" access delimiter for MotorcycleTagParser and road_access EV. * Move logic for access delimiter handling for ";" from RoadAccess into OSMRoadAccessParser * Fix for OSMRoadAccessParserTest * map match without graphhopper class (graphhopper#2657) * Solve Viterbi problem with Dijkstra to calculate fewer routes * Solve Viterbi problem with Dijkstra to calculate fewer routes * use 6.x/6.0 in readme * announcement * fix critical bug in client-hc for Matrix API * SBI Algo Co-authored-by: Peter <graphhopper@gmx.de> Co-authored-by: easbar <easbar.mail@posteo.net> Co-authored-by: otbutz <tbutz@optitool.de> Co-authored-by: Thomas Butz <thomas.butz@optitool.de> Co-authored-by: Michael Zilske <michael.zilske@tu-berlin.de> Co-authored-by: Robin <boldtrn@users.noreply.github.com> Co-authored-by: Andi <contactammmagamma@gmail.com> Co-authored-by: Rafael Sanches Telles <sanchesetelles@gmail.com> Co-authored-by: Rafael Telles <rafael@bzion.com.br> Co-authored-by: Alexey Abel <dev@abelonline.de> Co-authored-by: Lukas Weber <32765578+lukasalexanderweber@users.noreply.github.com> Co-authored-by: ratrun <ratrun@gmx.at>

karussell added 3 commits June 13, 2022 12:18

fix plateau-end check

302f66b

Merge branch 'master' into alt_fix

e25d939

Revert "Disable edge+alt measurements, they take too long"

87e5f8a

This reverts commit fb39266.

karussell added the performance label Jun 13, 2022

karussell added this to the 6.0 milestone Jun 13, 2022

karussell commented Jun 13, 2022

View reviewed changes

easbar reviewed Jun 13, 2022

View reviewed changes

tools/src/main/java/com/graphhopper/tools/Measurement.java Outdated Show resolved Hide resolved

easbar reviewed Jun 13, 2022

View reviewed changes

tools/src/main/java/com/graphhopper/tools/Measurement.java Outdated Show resolved Hide resolved

increase count to 10 for Measurement

20ed6b3

karussell changed the title ~~The non-CH alternative algorithm has a bug~~ Fix bug in non-CH AlternativeRoute Jun 13, 2022

karussell merged commit 8dafafb into master Jun 15, 2022

karussell deleted the alt_fix branch June 15, 2022 07:49

karussell added a commit that referenced this pull request Jun 15, 2022

AlternativeRoute: follow up refactoring, #2603

20c60a9

karussell added a commit that referenced this pull request Jun 15, 2022

Measurement: reduce routingCH_alt after it was accidentally raised in #…

05e3556

…2603

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in non-CH AlternativeRoute #2603

Fix bug in non-CH AlternativeRoute #2603

karussell commented Jun 13, 2022 •

edited

Loading

karussell Jun 13, 2022

karussell Jun 13, 2022

karussell Jun 13, 2022

easbar commented Jun 13, 2022

karussell commented Jun 16, 2022 •

edited

Loading

Fix bug in non-CH AlternativeRoute #2603

Fix bug in non-CH AlternativeRoute #2603

Conversation

karussell commented Jun 13, 2022 • edited Loading

karussell Jun 13, 2022

Choose a reason for hiding this comment

karussell Jun 13, 2022

Choose a reason for hiding this comment

karussell Jun 13, 2022

Choose a reason for hiding this comment

easbar commented Jun 13, 2022

karussell commented Jun 16, 2022 • edited Loading

karussell commented Jun 13, 2022 •

edited

Loading

karussell commented Jun 16, 2022 •

edited

Loading