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.

Sign up for GitHub

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

Jump to bottom

some bug fixes ported over from #4638 #4642

Merged

nilsnolde merged 6 commits into master from nn-pruning-bug-fixes

Mar 25, 2024

Member

nilsnolde commented Mar 20, 2024

more inline


          some bug fixes ported over from #4638

274400a

nilsnolde commented

View reviewed changes

src/sif/pedestriancost.cc Outdated

@@ @@ -550,7 +550,6 @@ class PedestrianCost : public DynamicCost { @@
                        (edge->use() == baldr::Use::kRailFerry && pred->use() != baldr::Use::kRailFerry);
                   // Additional penalties without any time cost
-                  c.cost += destination_only_penalty_ * (edge->destonly() && !pred->destonly());

Member Author

nilsnolde Mar 20, 2024

ped with destonly penalty sounds weird:)

Member Author

nilsnolde Mar 22, 2024

hm, maybe it has a point since we also turn private roads into destination_only and even pedestrian shouldn't take those..

Member Author

nilsnolde Mar 22, 2024

it's a bit unfortunate though, "residents only" is pointless for ped but we can't distinguish

nilsnolde commented

View reviewed changes

src/sif/recost.cc

@@ @@ -91,7 +91,7 @@ void recost_forward(baldr::GraphReader& reader, @@
                   // TODO: if this edge begins a restriction, we need to start popping off edges into queue
                   // so that we can find if we reach the end of the restriction. then we need to replay the
                   // queued edges as normal
-                  uint8_t time_restrictions_TODO = -1;
+                  uint8_t time_restrictions_TODO = baldr::kInvalidRestriction;

Member Author

nilsnolde Mar 20, 2024

this you'll see a lot, why not use it if it already exists.

nilsnolde commented

View reviewed changes

src/thor/astar_bss.cc

                   // Add to the adjacency list and edge labels.
                   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, InternalTurn::kNoTurn,

Member Author

nilsnolde Mar 20, 2024

also this you'll see a lot: this flag is setting closure_pruning which is not smth we want for lots of algos (not that it matters at all, but it's again more explicit IMO to set false here)

nilsnolde commented

View reviewed changes

src/thor/bidirectional_astar.cc

                                         : astarheuristic_reverse_.Get(t2->get_node_ll(meta.edge->endnode()), dist));
                 // not_thru_pruning_ is only set to false on the 2nd pass in route_action.
-                bool thru = not_thru_pruning_ ? (pred.not_thru_pruning() || !meta.edge->not_thru()) : false;
+                // We allow settling not_thru edges so we can connect both trees on them.

Member Author

nilsnolde Mar 20, 2024

this comment is what eventually wasted a good 2 days of work on #4638 when I discovered that at the very end. very subtle..

nilsnolde commented

View reviewed changes

src/thor/bidirectional_astar.cc

    
                                                   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)),

Member Author

nilsnolde Mar 20, 2024

I'm 100% sure now that we need to look at the opposing edge here because we're expanding on the reverse tree. case in point: AllowedReverse will prune on the opposing edge as well, not the one that gets expanded. it's really just common sense, though easy to miss..

you'll see that in a lot of ExpandInner functions throughout the PR

nilsnolde commented

View reviewed changes

src/thor/bidirectional_astar.cc Outdated Show resolved Hide resolved

nilsnolde commented

View reviewed changes

src/thor/bidirectional_astar.cc

                   // 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();

Member Author

nilsnolde Mar 20, 2024

just putting this in a variable to make it clearer

nilsnolde commented

View reviewed changes

src/thor/bidirectional_astar.cc

    
                                                !edgelabels_forward_.back().not_thru_pruning();

                  pruning_disabled_at_origin_ =

                      pruning_disabled_at_origin_ || !edgelabels_forward_.back().closure_pruning() ||

                      !edgelabels_forward_.back().not_thru_pruning() || edgelabels_forward_.back().destonly();

Member Author

nilsnolde Mar 20, 2024

if the destination/origin projects on destonly edge, it's also a reason to extend expansion in the opposite direction.

nilsnolde commented

View reviewed changes

src/thor/bidirectional_astar.cc

Comment on lines +1115 to +1116

		directededge->destonly() \|\|
		(costing_->is_hgv() && directededge->destonly_hgv()));

Member Author

nilsnolde Mar 20, 2024

however, in SetDestination functions we enqueue the opposing edge, so really we need to look at the correlated edge here

nilsnolde commented

View reviewed changes

src/thor/costmatrix.cc

-                // TODO(nils): one of these cases where I think reverse tree should look at the opposing edge,
-                //   not the expanding one, same for quite some attributes below (and same in bidir a*)
-                bool thru = not_thru_pruning_ ? (pred.not_thru_pruning() || !meta.edge->not_thru()) : false;
+                // We allow settling not_thru edges so we can connect both trees on them.

Member Author

nilsnolde Mar 20, 2024

changes in CostMatrix are basically the exact same as in bidir A*

nilsnolde commented

View reviewed changes

src/thor/timedistancebssmatrix.cc

                     dist = static_cast<uint32_t>(directededge->length() * percent_along);
                   } else {
-                    opp_edge_id = graphreader.GetOpposingEdgeId(edgeid);
+                    opp_edge_id = graphreader.GetOpposingEdgeId(edgeid, endtile);

Member Author

nilsnolde Mar 20, 2024

just some caching stuff

nilsnolde commented

View reviewed changes

src/thor/timedistancematrix.cc

-                                           static_cast<bool>(flow_sources & kDefaultFlowMask), turn_type, 0,
-                                           directededge->destonly() ||
-                                               (costing_->is_hgv() && directededge->destonly_hgv()));
+                  if (FORWARD) {

Member Author

nilsnolde Mar 20, 2024

this I had to take apart because we're exapnding here and need to look at the opposing edge for the reverse tree to understand if it IsClosed(), but also for the destonly stuff

nilsnolde commented

View reviewed changes

src/thor/unidirectional_astar.cc Outdated

                 midgard::PointLL destination_new(destination.correlation().edges(0).ll().lng(),
                                                  destination.correlation().edges(0).ll().lat());
                 Init(origin_new, destination_new);
-                float mindist = astarheuristic_.GetDistance(origin_new);
+                float mindist = astarheuristic_.GetDistance(FORWARD ? origin_new : destination_new);

Member Author

nilsnolde Mar 20, 2024

this seemed like another bug. on reverse we want to get the mindist from the origin to the destination

nilsnolde force-pushed the nn-pruning-bug-fixes branch from b0662ec to 039fa9d Compare

March 20, 2024 15:17


          minor fixes

039fa9d

nilsnolde commented

View reviewed changes

valhalla/sif/costfactory.h

@@ @@ -101,6 +101,7 @@ class CostFactory { @@
                 mode_costing_t CreateModeCosting(const Options& options, TravelMode& mode) {
                   mode_costing_t mode_costing;
+                  mode = TravelMode::kMaxTravelMode;

Member Author

nilsnolde Mar 20, 2024

some of the tests were all of a sudden throwing a "mode might be uninitialized" warning. it's technically possible I guess, in case we'd happen to add another costing type which isn't represented in kCostingTypeMapping. makes me think of #4492, in case you're not yet aware of it @ianthetechie :)

Contributor

ianthetechie Mar 20, 2024

heh ;) Thanks for the heads up @nilsnolde. This is vaguely familiar but making a note to review when I get back to finalizing that PR.

nilsnolde requested review from dnesbitt61, kevinkreiser and gknisely

March 20, 2024 15:22


          changelog

f20fdd5

This was referenced Mar 22, 2024

truck penalty for hgv=no edges #4650

Merged

FIXED Issue #4652: src/thor/unidirectional_astar.cc doesnt work for date_time type 2, i.e. for arrival #4653

Merged

nilsnolde added 3 commits

March 25, 2024 14:02


          Update pedestriancost.cc

c8a557c


          Update pedestriancost.cc

8387b1b


          Merge branch 'master' into nn-pruning-bug-fixes

910c1ed

kevinkreiser approved these changes

View reviewed changes

Member

kevinkreiser left a comment

should do a quick rad on these

Member Author

nilsnolde commented Mar 25, 2024

Did the RAD test for de_benchmark_routes.txt and it's the same in perf & results for the 20ish routes I compared.

I guess the only thing which could have a "negative" impact is adding a location's destonly as a criterion to extend the search in the other direction, but really that's an improvement, it might save us from a second pass.

nilsnolde merged commit 962719a into master

8 of 9 checks passed

nilsnolde deleted the nn-pruning-bug-fixes branch

March 25, 2024 15:10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment