Now threshold in shortest paths is applied on the whole path, not ste…#438
Now threshold in shortest paths is applied on the whole path, not ste…#438
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #438 +/- ##
==========================================
- Coverage 87.26% 87.09% -0.17%
==========================================
Files 52 52
Lines 6523 6554 +31
Branches 732 732
==========================================
+ Hits 5692 5708 +16
- Misses 809 822 +13
- Partials 22 24 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| graph.addStreets(s1, s2, s3, s4, s5); | ||
|
|
||
| auto const& pathMap = | ||
| graph.allPathsTo(4, [](auto const& pEdge) { return pEdge.length(); }, 0.7); |
|
|
||
| auto const& pathMap = | ||
| graph.allPathsTo(4, [](auto const& pEdge) { return pEdge.length(); }, 0.7); | ||
| CHECK_EQ(pathMap.at(0).size(), 2); |
| auto const& pathMap = | ||
| graph.allPathsTo(4, [](auto const& pEdge) { return pEdge.length(); }, 0.7); | ||
| CHECK_EQ(pathMap.at(0).size(), 2); | ||
| } |
| graph.allPathsTo(4, [](auto const& pEdge) { return pEdge.length(); }, 0.7); | ||
| CHECK_EQ(pathMap.at(0).size(), 2); | ||
| } | ||
|
|
| // Best path: 0 -> 1 -> 2 -> 5 = 10.0 | ||
| // Alternative: 0 -> 1 -> 3 -> 5 = 10.89 (within 10%) | ||
| // Over-budget path: 0 -> 1 -> 3 -> 4 -> 5 = 11.39 (must be excluded) | ||
| Street s01(0, std::make_pair(0, 1), 1.0); |
| path[3] == 5) { | ||
| foundBestPath = true; | ||
| } | ||
| if (path.size() == 4 && path[0] == 0 && path[1] == 1 && path[2] == 3 && |
| path[3] == 5) { | ||
| foundValidAlternative = true; | ||
| } | ||
| if (path.size() == 5 && path[0] == 0 && path[1] == 1 && path[2] == 3 && |
| path[3] == 4 && path[4] == 5) { | ||
| foundOverBudgetPath = true; | ||
| } | ||
| } |
|
|
||
| CHECK(foundBestPath); | ||
| CHECK(foundValidAlternative); | ||
| CHECK_FALSE(foundOverBudgetPath); |
| CHECK(foundBestPath); | ||
| CHECK(foundValidAlternative); | ||
| CHECK_FALSE(foundOverBudgetPath); | ||
| } |
There was a problem hiding this comment.
Pull request overview
This PR updates the road-network pathfinding “threshold” semantics so the tolerance is applied against the total source→target (or node→target) path cost, rather than being applied step-by-step at each hop. This affects both C++ pathfinding behavior and its test coverage, plus a small Python-docstring clarification.
Changes:
- Refactors
RoadNetwork::allPathsTo()/RoadNetwork::shortestPath()to compute strict distances-to-target first, then filter next-hops using a full-path budget rule. - Adds new tests covering deeper alternative paths and ensuring over-budget full paths are excluded.
- Updates
FirstOrderDynamicstest usage and Python binding docstrings to match the clarified threshold meaning.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/dsf/mobility/RoadNetwork.hpp |
Refactors pathfinding to enforce full-path budget threshold via distances-to-target and new filtering/recursion logic. |
test/mobility/Test_graph.cpp |
Adds new unit tests for full-path threshold behavior (deep alternatives + budget exclusion). |
test/mobility/Test_dynamics.cpp |
Adjusts weight-function threshold usage in a dynamics test to avoid unintended alternatives under new semantics. |
src/dsf/bindings.cpp |
Clarifies Python docstring: threshold is relative tolerance on full source→target cost. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Keep recursion acyclic and guaranteed to converge toward target. | ||
| if (nextDistToTarget + detail::kPathBudgetEpsilon >= currentDistToTarget) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Both allPathsTo() and shortestPath() drop any hop where distToTarget[next] >= distToTarget[current] (the “acyclic / converge” check). That enforces a strictly distance-decreasing property, which is stronger than the documented “within threshold budget” semantics and will exclude valid within-budget detours that temporarily move to a node with larger distToTarget.
If this monotonic constraint is intentional (e.g., to keep PathCollection::explode finite), it should be documented on the APIs; otherwise consider a different cycle-prevention mechanism (e.g., path-local visited set during expansion).
| std::function<bool(Id, double)> buildPathsWithinBudget; | ||
| buildPathsWithinBudget = [&](Id const currentNode, double const prefixCost) -> bool { | ||
| if (currentNode == targetId) { | ||
| return true; | ||
| } | ||
|
|
||
| while (!queue.empty()) { | ||
| Id current = queue.front(); | ||
| queue.pop(); | ||
| auto const currentDistToTarget = distToTarget.at(currentNode); | ||
| bool hasValidPathToTarget{false}; | ||
|
|
||
| if (current == targetId) { | ||
| continue; | ||
| } | ||
| for (auto const& outEdgeId : this->node(currentNode).outgoingEdges()) { | ||
| auto const& outEdge = this->edge(outEdgeId); | ||
| if (outEdge.roadStatus() == RoadStatus::CLOSED) { | ||
| continue; | ||
| } | ||
|
|
||
| auto const nextNodeId = outEdge.target(); | ||
| auto const nextDistToTarget = distToTarget.at(nextNodeId); | ||
| if (nextDistToTarget == std::numeric_limits<double>::infinity()) { | ||
| continue; | ||
| } | ||
|
|
||
| // Add this node's next hops to the result if they exist | ||
| if (nextHopsToTarget.contains(current) && !nextHopsToTarget[current].empty()) { | ||
| result[current] = nextHopsToTarget[current]; | ||
| // Keep recursion acyclic and guaranteed to converge toward target. | ||
| if (nextDistToTarget + detail::kPathBudgetEpsilon >= currentDistToTarget) { | ||
| continue; | ||
| } | ||
|
|
||
| auto const edgeWeight = f(outEdge); | ||
| auto const newPrefixCost = prefixCost + edgeWeight; | ||
| auto const optimisticCost = newPrefixCost + nextDistToTarget; | ||
| if (optimisticCost > sourceBudget + detail::kPathBudgetEpsilon) { | ||
| continue; | ||
| } | ||
|
|
||
| // Add next hops to the queue if not already visited | ||
| for (Id nextHop : nextHopsToTarget[current]) { | ||
| if (!nodesOnPath.contains(nextHop)) { | ||
| nodesOnPath.insert(nextHop); | ||
| queue.push(nextHop); | ||
| if (buildPathsWithinBudget(nextNodeId, newPrefixCost)) { | ||
| auto& hops = result[currentNode]; | ||
| if (std::find(hops.begin(), hops.end(), nextNodeId) == hops.end()) { | ||
| hops.push_back(nextNodeId); | ||
| } | ||
| hasValidPathToTarget = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return hasValidPathToTarget; | ||
| }; | ||
|
|
||
| (void)buildPathsWithinBudget(sourceId, 0.0); |
There was a problem hiding this comment.
shortestPath() uses a recursive lambda (buildPathsWithinBudget) to traverse the graph. On large/long road networks this can create very deep recursion (depth ~= path length), which risks stack overflow and is hard to debug operationally.
Consider switching to an explicit stack/iterative DFS over the dist-decreasing DAG (or otherwise bounding recursion depth).
| RoadNetwork graph{}; | ||
| graph.addStreets(s1, s2, s3, s4, s5); | ||
|
|
||
| auto const& pathMap = |
There was a problem hiding this comment.
allPathsTo() returns by value, but this test binds it to auto const& (a reference to a temporary). While lifetime-extension makes this safe, it’s easy to misread and can lead to accidental dangling refs if the code is refactored.
Prefer storing the result by value (auto const pathMap = ...) here.
| auto const& pathMap = | |
| auto const pathMap = |
| std::function<bool(Id, double)> buildPathsWithinBudget; | ||
| buildPathsWithinBudget = [&](Id const currentNode, double const prefixCost) -> bool { | ||
| if (currentNode == targetId) { | ||
| return true; | ||
| } | ||
|
|
||
| while (!queue.empty()) { | ||
| Id current = queue.front(); | ||
| queue.pop(); | ||
| auto const currentDistToTarget = distToTarget.at(currentNode); | ||
| bool hasValidPathToTarget{false}; | ||
|
|
||
| if (current == targetId) { | ||
| continue; | ||
| } | ||
| for (auto const& outEdgeId : this->node(currentNode).outgoingEdges()) { | ||
| auto const& outEdge = this->edge(outEdgeId); | ||
| if (outEdge.roadStatus() == RoadStatus::CLOSED) { | ||
| continue; | ||
| } | ||
|
|
||
| auto const nextNodeId = outEdge.target(); | ||
| auto const nextDistToTarget = distToTarget.at(nextNodeId); | ||
| if (nextDistToTarget == std::numeric_limits<double>::infinity()) { | ||
| continue; | ||
| } | ||
|
|
||
| // Add this node's next hops to the result if they exist | ||
| if (nextHopsToTarget.contains(current) && !nextHopsToTarget[current].empty()) { | ||
| result[current] = nextHopsToTarget[current]; | ||
| // Keep recursion acyclic and guaranteed to converge toward target. | ||
| if (nextDistToTarget + detail::kPathBudgetEpsilon >= currentDistToTarget) { | ||
| continue; | ||
| } | ||
|
|
||
| auto const edgeWeight = f(outEdge); | ||
| auto const newPrefixCost = prefixCost + edgeWeight; | ||
| auto const optimisticCost = newPrefixCost + nextDistToTarget; | ||
| if (optimisticCost > sourceBudget + detail::kPathBudgetEpsilon) { | ||
| continue; | ||
| } | ||
|
|
||
| // Add next hops to the queue if not already visited | ||
| for (Id nextHop : nextHopsToTarget[current]) { | ||
| if (!nodesOnPath.contains(nextHop)) { | ||
| nodesOnPath.insert(nextHop); | ||
| queue.push(nextHop); | ||
| if (buildPathsWithinBudget(nextNodeId, newPrefixCost)) { | ||
| auto& hops = result[currentNode]; | ||
| if (std::find(hops.begin(), hops.end(), nextNodeId) == hops.end()) { | ||
| hops.push_back(nextNodeId); | ||
| } | ||
| hasValidPathToTarget = true; |
There was a problem hiding this comment.
shortestPath() builds a node->nextHops map while the feasibility check depends on prefixCost (how you reached the node). Because result[currentNode] is shared across all recursive visits, next-hops that are only valid for a cheaper prefix can remain in the map and later be combined (via PathCollection::explode) with a more expensive prefix path to produce a source→target path that exceeds sourceBudget.
To make the returned PathCollection sound, consider constraining stored edges so they’re valid for any reachable prefix in the returned graph (e.g., restrict to edges consistent with a distFromSource DAG, or redesign to return explicit paths / include prefix-state).
Grufoony
force-pushed
the
fixThresholdInPaths
branch
from
f1e9f1a to
5adaeba
Compare
…p-by-step