Improve TSP with different vehicle start/end locations#1254
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extends genetic algorithm recombination and local search to support TSP path problems (fixed start/end locations) by introducing virtual-depot normalization in EAX, linear-path offspring generation in OX, and updating candidate enumeration and node-info extraction in sliding TSP search. ChangesTSP Path Problem Support in Genetic Algorithm Components
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/routing/local_search/sliding_tsp.cu (1)
320-328:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDon't resolve
NodeInfofrom mutable route indices.
cand.window_startandcand.insertion_posare based on the original route layout, but earlier accepted slides can shift unrelated indices without marking that whole span inmoved_regions. After that,s_route.get_node(cand.*)can refer to a different node thanoriginal_*_node_id, so these assertions can trip on valid candidate sets, or the rewiring can use the wrong endpoints if assertions are compiled out. Please keep the stable node ids frommoved_regionsas the source of truth here and resolveNodeInfofrom a node-id lookup that also preserves depot vs. order node types.As per coding guidelines, "Flag algorithm correctness errors including logic errors in optimization algorithms".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/routing/local_search/sliding_tsp.cu` around lines 320 - 328, The assertions and NodeInfo resolution must not use mutable route indices (s_route.get_node(cand.window_start) / get_node(cand.insertion_pos)); instead, look up NodeInfo by the stable node ids recorded in moved_regions (e.g. use the stored original_*_node_id values) so you always resolve the same node even if prior accepted slides shifted route indices; replace the get_node calls used to build original_node_info, original_fragment_end_node_info, and original_node_insertion_info with a node-id based lookup that preserves depot vs. order node types and then assert those NodeInfo.node() values against original_node_id, original_fragment_end_node_id, and original_node_insertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpp/src/routing/local_search/sliding_tsp.cu`:
- Around line 320-328: The assertions and NodeInfo resolution must not use
mutable route indices (s_route.get_node(cand.window_start) /
get_node(cand.insertion_pos)); instead, look up NodeInfo by the stable node ids
recorded in moved_regions (e.g. use the stored original_*_node_id values) so you
always resolve the same node even if prior accepted slides shifted route
indices; replace the get_node calls used to build original_node_info,
original_fragment_end_node_info, and original_node_insertion_info with a node-id
based lookup that preserves depot vs. order node types and then assert those
NodeInfo.node() values against original_node_id, original_fragment_end_node_id,
and original_node_insertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9ff553ae-0585-45e2-b1c6-0da86e5faf46
📒 Files selected for processing (1)
cpp/src/routing/local_search/sliding_tsp.cu
| NodeInfo<i_t>(original_node_insertion, | ||
| sol.problem.order_info.get_order_location(original_node_insertion), | ||
| node_type_t::DELIVERY); | ||
| original_node_insertion == start_depot_node_info.node() |
There was a problem hiding this comment.
The node value for the depot is undefined, I think
| bool use_linear_ox_for_path_tsp(Solution const& S, int n_routes) const | ||
| { | ||
| if (!S.problem->is_tsp || n_routes != 1) { return false; } | ||
| if (S.problem->data_view_ptr->get_vehicle_locations().first == nullptr) { return false; } |
There was a problem hiding this comment.
What happens if the vehicle start location and end locations are same, but they are set by the user?
|
|
||
| detail::NodeInfo<> normalize_depot(detail::NodeInfo<> node) const | ||
| { | ||
| if (use_virtual_depot_node && node.is_depot()) { return single_depot_node.value(); } |
There was a problem hiding this comment.
Can you explain the logic here?
2 participants
Improve routing support for TSP models that use
set_vehicle_locations()with distinct start andend locations. These models are fixed paths, not closed tours, so some TSP-specific operators need
to avoid cyclic assumptions around the start/end boundary.
Add path-aware EAX handling for fixed-start/fixed-end TSPs:
Add path-aware OX handling:
fill_offspring(..., linear_path_tsp)to reduce duplicate helper code.Update sliding TSP local search
nodes_to_considerAdd a unit test covering TSP with order locations plus vehicle start/end locations.