refactor path pruning for not_thru/destonly/closures #4638
Conversation
| @@ -423,7 +423,8 @@ bool AutoCost::Allowed(const baldr::DirectedEdge* edge, | |||
| if (!IsAccessible(edge) || (!pred.deadend() && pred.opp_local_idx() == edge->localedgeidx()) || | |||
| ((pred.restrictions() & (1 << edge->localedgeidx())) && !ignore_turn_restrictions_) || | |||
| edge->surface() == Surface::kImpassable || IsUserAvoidEdge(edgeid) || | |||
| (!allow_destination_only_ && !pred.destonly() && edge->destonly()) || | |||
| (destination_only_pruning_ && pred.destonly_pruning() && edge->destonly()) || | |||
| (not_thru_pruning_ && pred.not_thru_pruning() && edge->not_thru()) || | |||
There was a problem hiding this comment.
I moved the not_thru check into Allowed(Reverse)
There was a problem hiding this comment.
that means it will NOT be put in the adjacency list and that is what's breaking the test below in gurka_route
src/sif/dynamiccost.cc
Outdated
| : pass_(0), allow_transit_connections_(false), allow_destination_only_(true), | ||
| allow_conditional_destination_(false), travel_mode_(mode), access_mask_(access_mask), | ||
| closure_factor_(kDefaultClosureFactor), flow_mask_(kDefaultFlowMask), | ||
| : pass_(0), allow_transit_connections_(false), destination_only_pruning_(true), |
There was a problem hiding this comment.
renamed the members to be consistent (now it has the opposite meaning of before, so I had to change all logic where this is used or set)
| uint32_t idx = edgelabels_.size(); | ||
| edgelabels_.emplace_back(pred_idx, edgeid, GraphId(), directededge, newcost, sortcost, dist, mode, | ||
| transition_cost, false, true, false, InternalTurn::kNoTurn, | ||
| transition_cost, false, false, false, false, InternalTurn::kNoTurn, |
There was a problem hiding this comment.
no pruning at all, see comment
| @@ -361,9 +361,8 @@ inline bool BidirectionalAStar::ExpandInner(baldr::GraphReader& graphreader, | |||
| pred.path_distance() + meta.edge->length(), newcost.cost); | |||
| } | |||
|
|
|||
| // we've just added this edge to the queue, but we won't expand from it if it's a not-thru edge that | |||
| // will be pruned. In that case we want to allow uturns. | |||
| return !(pred.not_thru_pruning() && meta.edge->not_thru()); | |||
There was a problem hiding this comment.
now we're not even adding not_thru edges to the queue, so this would be moot
| @@ -422,14 +421,14 @@ void BidirectionalAStar::Expand(baldr::GraphReader& graphreader, | |||
| // If so, it means we are attempting a u-turn. In that case, lets wait with evaluating | |||
| // this edge until last. If any other edges were emplaced, it means we should not | |||
| // even try to evaluate a u-turn since u-turns should only happen for deadends | |||
| uturn_meta = pred.opp_local_idx() == meta.edge->localedgeidx() ? meta : uturn_meta; | |||
| bool is_uturn = pred.opp_local_idx() == meta.edge->localedgeidx(); | |||
There was a problem hiding this comment.
just added this bool, we need it twice
| @@ -709,8 +708,7 @@ BidirectionalAStar::GetBestPath(valhalla::Location& origin, | |||
|
|
|||
| // Prune path if predecessor is not a through edge or if the maximum | |||
| // number of upward transitions has been exceeded on this hierarchy level. | |||
| if ((fwd_pred.not_thru() && fwd_pred.not_thru_pruning()) || | |||
There was a problem hiding this comment.
a settled edge can't be not_thru anymore
| !edgelabels_forward_.back().not_thru_pruning(); | ||
| pruning_disabled_at_origin_ = | ||
| pruning_disabled_at_origin_ || !edgelabels_forward_.back().closure_pruning() || | ||
| directededge->not_thru() || !edgelabels_forward_.back().destonly_pruning(); |
There was a problem hiding this comment.
destonly_pruning is also a valid condition to extend the search in the other direction
| sortcost, dist, mode_, transition_cost, thru, | ||
| (pred.closure_pruning() || !costing_->IsClosed(meta.edge, tile)), | ||
| sortcost, dist, mode_, transition_cost, not_thru_pruning, | ||
| pred.closure_pruning() || !(costing_->IsClosed(opp_edge, t2)), |
There was a problem hiding this comment.
it's easy to miss: I changed to checking if opp_edge is closed when expanding reverse because that's the one we want to go on in the final path
There was a problem hiding this comment.
there's quite a few of these fixes sprinkled in this PR
src/thor/bidirectional_astar.cc
Outdated
| dist, mode_, c, !opp_dir_edge->not_thru(), | ||
| !(costing_->IsClosed(directededge, tile)), | ||
| dist, mode_, c, false, !(costing_->IsClosed(directededge, tile)), | ||
| !(directededge->destonly() || |
There was a problem hiding this comment.
when setting the destination however, we want to look at the passed directed edge (not the opposing), because we put the opposing in the queue
| @@ -70,24 +70,24 @@ class EdgeLabel { | |||
| const TravelMode mode, | |||
| const uint32_t path_distance, | |||
| const uint8_t restriction_idx, | |||
| const bool not_thru_pruning, | |||
There was a problem hiding this comment.
I added both not_thru_pruning and destonly_pruning as first-class citizens to EdgeLabel, also timedistancematrix wants to look at them I think
| @@ -526,6 +526,7 @@ TEST(Standalone, BridgingEdgeIsRestricted) { | |||
| TEST(Standalone, AvoidExtraDetours) { | |||
| // Check that we don't take extra detours to get the target point. Here is | |||
| // a special usecase that breaks previous logic about connection edges in bidirectional astar. | |||
| // TODO(nils): this currently fails because both trees hit a not_thru area on DE (fwd) and DC(rev) | |||
There was a problem hiding this comment.
so in the map below, the route wants to go from 1 -> 2 with bidir a*. the test sets D -> A edges and D -> F edges as not_thru, which means neither can the fwd search connect to the D -> F edges of the rev tree, nor can the rev search connect to the D -> A edges of the fwd tree, so both trees have to expand into the HIJK region, connecting on a massive detour.
I think it fails now because we don't even record not_thru edges anymore while expanding which means a connection on those can't happen anymore.
2 things:
- this test describes a real failure mapbox noticed on their master a while back, but jeez this is kinda rare.. and not sure it really justifies the extra complex logic that was there before
- but ok, it shows a rare bug. however, the real problem here is that we do find a connection, it's just a huge detour. if we wouldn't find a connection in the first pass, that's fine, we would find it on a second pass. anyways, obviously it takes
CD&DEedges in either case. in master it'd make that transition directly to have the path{ "AB", "BC", "CD", "DE", "EF" }. in this PR, it can't and instead the path loops back on itself viaD, i.e. with path{ "AB", "BC", "CD", "DH", "HI", "IJ", "JK", "KH", "DH", "DE", "EF" }which simply adds the sub-path"DH", "HI", "IJ", "JK", "KH", "DH"in betweenCDandDE. now what I wonder: couldn't we detect that while recosting or in triplegbuilder and just cut that completely superfluous part out of the final path? tbh, I thought I saw smth like this before where the path had to make a detour due to some turn restriction and it'd cut out part of the final path which was just a superfluous loop..
There was a problem hiding this comment.
actually I'm pretty sure this could fairly easily be done. the only thing to be wary about that I can think of are turn restrictions. if in the case above CD -> DE was restricted, we'd allow the detour, otherwise we'd cut the superfluous part and recost the remainder of the path accordingly. it'll need to be recorded while recost_forward is running, I'll try an implementation later.
it's only interesting for bidirectional algorithms. and while it'll be a bit more of a burden since we'd expand that detour where we wouldn't have to, this should be a very rare case and would maybe help other cases of unnecessary detours as well?!
|
isochrones fail as well, the polygons changed their shape again with this PR, I'm not sure why yet, actually not much changed for dijkstra. but the IMO more important tests dedicated to |
nilsnolde
force-pushed
the
nn-refactor-prunings
branch
3 times, most recently
from
8385188
to
d990027
Compare
…_cost relies on it
…ther the algorithm, as it's really an algo decision.. needed to alter thor_worker::parse_costing to make this work, which is less than ideal. ultimately I'm ok with the result..
| // not_thru is the same for both trees | ||
| bool not_thru_pruning = pred.not_thru_pruning() || !meta.edge->not_thru(); |
There was a problem hiding this comment.
in fact, this algorithm never used not_thru pruning.. it recorded it in the label, but didn't make use of it.
…ional_restrictions test which uses unidir a* which previously wasn't respecting not_thru regions and now goes into a second pass where both conditional access restrictions AND not_thru are disabled and finds of course the shorter path right over the restricted edge
| // TODO: this triggers a not_thru pruning failing the expected route, which wasn't looked at before in | ||
| // unidir a* and now we enter a second pass in which the _both_ the not_thru pruning and conditional | ||
| // restriction are dismissed | ||
| TEST_F(ConditionalRestrictions, DISABLED_DestinationRestrictionOnMidEdgeIsValid_ManyAlternatives) { |
There was a problem hiding this comment.
this is unfortunate but also exposed a bug in the previous behavior of unidir a* where it was expanding into not_thru regions (or maybe it was wanted that way because it's unidirectional?). since it now does not expand into those regions, it fails on a first pass and on the second pass it finds the fastest route, right over the conditional restriction.
that behavior is really unfortunate.. and brings up more considerations.. not_thru is only a performance technique, but restrictions are legal constructs. it sucks a bit that we conflate both into a second pass.
I can think of 2 things to improve:
- actually do set not_thru for all edges in region #3812 where we set all edges in a not_thru region to
not_thru. the hope is that we wouldn't need a second pass at all, because we could accurately determine if a location is inside such a region and if so, we allow the other (or only) tree to expand into them, which would be surely better than wasting a full expansion and go into a second pass. however, I didn't fully investigate all potential side effect, so TBD.. - the fact that there's a not_thru edge somewhere in that test map is far from obvious.. and very surely will trip another future test map (and possibly does so already without noticing bcs of second passes?). I'd propose to turn all edge into
not_thru = falseby default and let tests who want it set the property explicitly.
|
ok, I'm down to 2 failing tests (urgh, not true, just seeing a matrix is failing too, but that's most likely due to an actual bug fix). both can be explained:
all in all: while this PR fixes a lot of subtle bugs, I kinda dug my own hole here trying to refactor the pruning things to make them more understandable. it's a LOT of changes for such a small-ish impact. even I'd try and abstract the concept a bit more in another PR, I'm not sure this is all worth it. also looking at the 2nd test failure, this PR is actually introducing a previously fixed bug. I'm proposing another solution to that particular bug which might solve other cases too, but again, might not be worth it.. hope we'll get some time to discuss this. |
nilsnolde
force-pushed
the
nn-refactor-prunings
branch
from
a3f15e0
to
7abd7f3
Compare
|
after talking to @dnesbitt61 about this, I'll put this on hold. the changes to not_thru pruning are not good (generating an already fixed bug) and my proposal to edit the final path is another potential hole with tons of problem cases I don't think of right now. I'll take out the small bug fixes from this PR and call it a day. refactoring this properly would take a lot of work it seems (more even than already put in here). he had another idea which seems worthwhile and could be investigated at some point: only turn off not_thru pruning once we get closer to the destination/origin on the fwd/rev tree. which seems intuitively correct and could save us from needing a second pass as well, especially coupled with being able to determine accurately if a dest/origin is inside such a region with #3812 (if not, never turn off pruning). |
nilsnolde
requested review from
dnesbitt61,
gknisely and
kevinkreiser
and removed request for
kevinkreiser,
dnesbitt61 and
gknisely
as preparation for #4568
in that issue I promised to look into the current path pruning conditions which we apply to origin/destination correlated edges to allow closed/destonly/not_thru, but otherwise prohibit them (and ignore some again on a 2nd pass). this is what I did:
not_thruis handled differently than the others and the special handling didn't make sense to me, so I refactored it to be consistent with the others. the caveat is that I put it on the costings (just like the other similar attributes), which is not really where it belongs, it's really an algorithm property (e.g. dijkstra/map-matching doesn't want that). so I had to alter the costing factory to accommodate that.. on the upside, now it's being checked inAllowed()alongside all other pruning techniques*EdgeLabela bit and making sure all algos & costings use those things properly and consistentlyhowever, eventually, after I refactored everything and made sure it's all sound, I ran a few existing tests and one particular one kinda makes my first refactor moot (treating
not_thruthe same as the others). the particular test that's failing is a quite a rare case but was added particularly to avoid the scenario where it's breaking on in this PR. however, we might be able to solve this in a different way rather than reverting my refactor (which does help a lot understanding things IMO). more comments on that test.