fixing incorrect pruning + backtrack handling in SearchTree#492
fixing incorrect pruning + backtrack handling in SearchTree#492robfitzgerald wants to merge 35 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the core SearchTree insertion semantics to avoid incorrect cross-label pruning at the same vertex (Issue #491), treating distinct label states as disjoint search-space nodes and replacing the prior pruning-based approach with an insertion-time behavior decision.
Changes:
- Remove label-dominance pruning (
search_pruning) and theLabelModel::compareAPI previously used for pruning. - Introduce
TrajectoryInsertBehaviorto decide whether to cancel, insert, or overwrite a node duringSearchTreeinsertion (insert_trajectory). - Update call sites and tests to use
insert_trajectory, and add/adjust tests covering overwrite/cancel and cycle detection.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/routee-compass/src/plugin/output/default/eval/ops.rs | Removes an unused test helper for compiled expressions. |
| rust/routee-compass-powertrain/src/model/charging/soc_label_model.rs | Drops label dominance comparison logic now that pruning is removed. |
| rust/routee-compass-core/src/model/termination/model.rs | Updates tests to use insert_trajectory and removes no-longer-needed imports. |
| rust/routee-compass-core/src/model/label/label_model.rs | Removes the compare method from the LabelModel trait. |
| rust/routee-compass-core/src/model/label/label_enum.rs | Changes U8StateVec equality/hash semantics (ignore padding via state_len). |
| rust/routee-compass-core/src/model/label/default/vertex_label_model.rs | Removes compare implementation consistent with updated LabelModel trait. |
| rust/routee-compass-core/src/algorithm/search/search_tree.rs | Replaces insert + pruning with insert_trajectory using TrajectoryInsertBehavior; simplifies label indexing. |
| rust/routee-compass-core/src/algorithm/search/search_tree_node.rs | Removes pruning-related child-count behavior (currently left partially commented). |
| rust/routee-compass-core/src/algorithm/search/search_pruning.rs | Deletes pruning implementation and its tests. |
| rust/routee-compass-core/src/algorithm/search/search_instance.rs | Updates to call insert_trajectory (no LabelModel passed to insertion). |
| rust/routee-compass-core/src/algorithm/search/search_algorithm.rs | Updates to call insert_trajectory. |
| rust/routee-compass-core/src/algorithm/search/mod.rs | Exports insert_behavior module and removes search_pruning export. |
| rust/routee-compass-core/src/algorithm/search/insert_behavior.rs | Adds TrajectoryInsertBehavior and insertion decision logic. |
| rust/routee-compass-core/src/algorithm/search/a_star/frontier_instance.rs | Updates tests to use insert_trajectory and adjusts imports (one pruning-related comment remains). |
| rust/routee-compass-core/src/algorithm/search/a_star/a_star_algorithm.rs | Updates insertion call to insert_trajectory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rust/routee-compass-core/src/algorithm/search/insert_behavior.rs
Outdated
Show resolved
Hide resolved
rust/routee-compass-core/src/algorithm/search/insert_behavior.rs
Outdated
Show resolved
Hide resolved
rust/routee-compass-core/src/algorithm/search/a_star/frontier_instance.rs
Outdated
Show resolved
Hide resolved
nreinicke
left a comment
There was a problem hiding this comment.
This looks good and I love that you've simplified this. I have a couple of comments but will approve so you can proceed with downstream usage.
There was a problem hiding this comment.
Now that we've simplified this, clippy caught that we can remove the outer loop in the pop_new method. We don't need to do this here but eventually we should add clippy checks into our rust actions.
There was a problem hiding this comment.
i think my rust toolchain just needed to be upgraded, i usually run clippy every time. but ya, i thought we were checking clippy in our CI workflow!
There was a problem hiding this comment.
I'm wondering if we should also remove the get_min_cost_label and min_cost_ordering functions since they return the label with the minimum edge cost and not accumulated cost which could be misleading. Or, if you need this for downstream work, perhaps we do need to add cumulative cost to our search tree node so we can get the proper label out of this function.
There was a problem hiding this comment.
this is tricky, this is also core to how we backtrack, as that starts from a VertexId, but we need to select the Label associated with that destination VertexId that is least cost, and then backtrack through that tree. we in fact do need this to be the accumulated cost for this selection to be correct. i'll get to work proposing a fix so that TraversalCost holds both edge_cost and total_cost (or trip_cost).
|
@nreinicke this is passing now, yay, but, it's a way different set of changes since your review. i will directly reference this branch for my development tasks next week so that you can take your time to review these major code changes. have a great conf! |
this PR set out to address #491, where nodes are being removed by a combination of maximizing state and minimizing cost. this has the effect of incorrectly managing subtree spaces, as the tree spaces associated with two different label states at the same vertex should not be treated as related in the search space. however, the impact of investigating this change led to the discovery of additional logic fixes.
1. Removed Tree Pruning for Simplified Insertion Logic
The complex
search_pruning.rsmodule and Pareto-dominance pruning logic were removed entirely.SearchTreeassumes testing for cost improvement occurred in the search algorithm. This significantly simplifies tree insertion while actively preventing cycles.2. Introduced
EdgeTraversalContextfor Traversal ModelsWe needed to add the parent Label of a traversal in order to know where to start any backtracking. This is a requirement of the TurnDelay TraversalModel. It was incorrectly grabbing the min cost label associated with the source Vertex of a traversal. by passing in the parent Label, we can index into the correct subtree.
This increased the number of arguments to traverse_edge. To clean up
TraversalModel::traverse_edgeandCostModelmethod signatures, the loose trajectory arguments(&Vertex, &Edge, &Vertex)and&SearchTreealong with the parentLabelwere bundled into a comprehensiveEdgeTraversalContext. This context also provides safe helper methods likebacktrack()andprevious_edge_traversal(), reducing boilerplate in models that look into the search history (likeTurnDelayTraversalModel).3. Refactored
TraversalCostand Enforced Algorithmic Cost ConstraintsTraversalCostwas revised to be more semantic:total_costwas renamed toedge_costto clarify that it defines the marginal cost of an edge—not the accumulated path cost. Additionally, aCostConstraintenum (StrictlyPositivevsUnconstrained) was wired from the top-levelSearchAlgorithmconfiguration down through theCostModeland intoTraversalCost::insert()to ensure algorithms like A*/Dijkstra safely enforce monotonic (positive) costs, leaving an opening for any algorithms in the Bellman-Ford family which do not require monotonicity.Minor Changes
SearchTree& Algorithms:calculate_total_objective_costtoSearchTreethat includes infinite-loop guards against malformed tree cycles.SearchTree::insert()toSearchTree::insert_trajectory().get_min_accumulated_cost_label()which calculates the true minimal route by walking back the objective cost of paths.child_countproperty fromSearchTreeNode.TraversalCost:total_costfield toedge_costsystem-wide.TraversalCost::default()andzero()with clearer explicit constructors likeTraversalCost::empty()and a#[cfg(test)] pub fn mock()helper.CostConstraintparameter intoTraversalCost::insert().TraversalModel&CostModelimplementations:DistanceTraversalModel,SpeedTraversalModel,TurnDelayTraversalModel,SimpleChargingModel, etc.) to useEdgeTraversalContextfor edge and vertex data access.NetworkCostRate::network_costto pull trajectory details directly fromEdgeTraversalContext.Closes #491.