refactor(routing): standardize cost terminology#1299
Open
Juan-Francisco-Robles wants to merge 3 commits into
Open
refactor(routing): standardize cost terminology#1299Juan-Francisco-Robles wants to merge 3 commits into
Juan-Francisco-Robles wants to merge 3 commits into
Conversation
All files referenced in this commit are part of the routing package of the cuOpt library. Replace distance naming with cost naming where the value represents the cuOpt cost matrix rather than physical distance. Rename distance route/node helpers to cost counterparts and update routing internals that consume those types. Affected routing files: - cpp/include/cuopt/routing/data_model_view.hpp - cpp/src/routing/arc_value.hpp - cpp/src/routing/dimensions.cuh - cpp/src/routing/diversity/helpers.hpp - cpp/src/routing/diversity/macros.hpp - cpp/src/routing/ges/lexicographic_search/lexicographic_search.cu - cpp/src/routing/ges/lexicographic_search/node_stack.cuh - cpp/src/routing/local_search/compute_compatible.cu - cpp/src/routing/local_search/permutation_helper.cuh - cpp/src/routing/local_search/sliding_tsp.cu - cpp/src/routing/local_search/sliding_window.cu - cpp/src/routing/local_search/two_opt.cu - cpp/src/routing/local_search/vrp/fragment_kernels.cuh - cpp/src/routing/local_search/vrp/vrp_search.cu - cpp/src/routing/node/cost_node.cuh - cpp/src/routing/node/node.cuh - cpp/src/routing/problem/problem.cu - cpp/src/routing/route/cost_route.cuh - cpp/src/routing/route/dimensions_route.cuh - cpp/src/routing/route/mismatch_route.cuh - cpp/src/routing/route/prize_route.cuh - cpp/src/routing/route/service_time_route.cuh - cpp/src/routing/route/tasks_route.cuh - cpp/src/routing/route/vehicle_fixed_cost_route.cuh - cpp/src/routing/util_kernels/set_nodes_data.cuh This improves clarity without changing route optimization behavior. Signed-off-by: Juan Francisco Robles <juanfrancisco.robles@oga.ai>
|
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 (8)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors the routing primary dimension from "distance" to "cost": enums, dispatch helpers, node/route storage, local-search kernels, shared-memory layouts, Doxygen, and unit tests updated to use COST and cost_* buffers. ChangesDimension System and Local Search Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labelsnon-breaking 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.
Actionable comments posted: 7
🧹 Nitpick comments (4)
cpp/src/routing/local_search/vrp/vrp_search.cu (1)
22-40: 💤 Low valueConsider renaming
compute_reverse_distancestocompute_reverse_costsfor terminology consistency.The kernel now computes and stores values in
cost_dim.reverse_costusingdim_t::COST, but the function name still references "distances". This is a minor inconsistency that could be addressed as a follow-up.🤖 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/vrp/vrp_search.cu` around lines 22 - 40, Rename the kernel compute_reverse_distances to compute_reverse_costs to match the fact it computes cost_dim.reverse_cost using dim_t::COST; update all references and declarations (e.g., the kernel definition template<typename i_t, typename f_t, request_t REQUEST> __global__ void compute_reverse_distances(typename solution_t<i_t, f_t, REQUEST>::view_t solution) and any kernel launches) and ensure the symbol name change is reflected where route.dimensions.cost_dim.reverse_cost and get_arc_of_dimension<i_t,f_t,dim_t::COST> are used so callers and tests compile against compute_reverse_costs.cpp/src/routing/ges/lexicographic_search/lexicographic_search.cu (2)
391-396: 💤 Low valueMinor: Same naming inconsistency as above.
The variable
is_delivery_time_dist_feasibleat line 392 should also be renamed for consistency.Suggested rename
if (forward_feasible) { - bool is_delivery_time_dist_feasible = + bool is_delivery_time_cost_feasible = node_stack.delivery_node.time_dim.forward_feasible( node_stack.s_route.vehicle_info()) && node_stack.delivery_node.cost_dim.forward_feasible(node_stack.s_route.vehicle_info()); - if (!is_delivery_time_dist_feasible) { + if (!is_delivery_time_cost_feasible) {🤖 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/ges/lexicographic_search/lexicographic_search.cu` around lines 391 - 396, Rename the local boolean is_delivery_time_dist_feasible to match the naming used elsewhere (e.g., is_delivery_time_and_dist_feasible) for consistency; update its declaration and all uses in the block where node_stack.delivery_node.time_dim.forward_feasible(...) and node_stack.delivery_node.cost_dim.forward_feasible(...) are checked (symbols: node_stack, delivery_node, time_dim, cost_dim, forward_feasible, s_route, vehicle_info) so the new name replaces the old one throughout that conditional.
311-316: 💤 Low valueMinor: Variable name
is_delivery_time_dist_feasibleis misleading after the refactor.The variable now checks
cost_diminstead ofdistance_dim, but the name still contains "dist". Consider renaming tois_delivery_time_cost_feasiblefor clarity.Suggested rename
if (is_forward_feasible) { - bool is_delivery_time_dist_feasible = + bool is_delivery_time_cost_feasible = node_stack.delivery_node.time_dim.forward_feasible( node_stack.s_route.vehicle_info()) && node_stack.delivery_node.cost_dim.forward_feasible(node_stack.s_route.vehicle_info()); - if (!is_delivery_time_dist_feasible) { + if (!is_delivery_time_cost_feasible) {🤖 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/ges/lexicographic_search/lexicographic_search.cu` around lines 311 - 316, The variable name is_delivery_time_dist_feasible is misleading because it now checks delivery_node.cost_dim rather than distance; rename it to is_delivery_time_cost_feasible wherever declared and referenced (the boolean assigned from node_stack.delivery_node.time_dim.forward_feasible(node_stack.s_route.vehicle_info()) && node_stack.delivery_node.cost_dim.forward_feasible(node_stack.s_route.vehicle_info()) and the subsequent if (!...) check) so the identifier matches the checked dimensions (refer to node_stack, delivery_node, time_dim, cost_dim, and s_route.vehicle_info()) and update all usages accordingly.cpp/src/routing/ges/lexicographic_search/node_stack.cuh (1)
213-218: 💤 Low valueMinor: Variable name
shared_for_dist_buffersis now misleading.The variable name still references "dist" but is now used for cost dimension buffer sizing.
Suggested rename
if (solution_ptr->problem_ptr->dimensions_info.cost_dim.has_constraints()) { - const size_t shared_for_dist_buffers = + const size_t shared_for_cost_buffers = (2 + (max_neighbors<i_t, REQUEST>(k_max) + 1)) * (solution_ptr->get_max_active_nodes_for_all_routes() + 1) * sizeof(f_t); - sh_size += shared_for_dist_buffers; + sh_size += shared_for_cost_buffers; }🤖 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/ges/lexicographic_search/node_stack.cuh` around lines 213 - 218, The local variable shared_for_dist_buffers is misnamed because it actually sizes buffers for the cost dimension; rename it to something clearer like shared_for_cost_buffers (or shared_for_cost_dim_buffers) where it's declared and used in the block guarded by solution_ptr->problem_ptr->dimensions_info.cost_dim.has_constraints(), and update any references (e.g., the sh_size += shared_for_dist_buffers line) to the new name so the intent (cost-dimension buffer sizing) is clear.
🤖 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.
Inline comments:
In `@cpp/src/routing/route/dimensions_route.cuh`:
- Around line 267-268: The comment above the `cost_dim` member is stale and
still says "distance route"; update that comment to reflect the correct
terminology (e.g., "cost route" or remove misleading problem-space reference)
next to the `cost_route_t<i_t, f_t> cost_dim;` declaration so the comment
matches the symbol name and coding guidelines about avoiding misleading
problem-space terms.
In `@cpp/src/routing/route/mismatch_route.cuh`:
- Line 141: The `@brief` on the helper that returns the shared-memory size is
misleadingly referring to a "cost route"; update the brief to state it computes
shared-memory size for a mismatch_route_t (the mismatch-route storage helper) so
the comment matches the actual type and purpose (refer to mismatch_route_t and
the shared-memory size helper in mismatch_route.cuh).
In `@cpp/src/routing/route/prize_route.cuh`:
- Line 151: Update the documentation comment for the shared-memory size helper
to use prize-specific wording: replace "cost route" with "prize route" (or
similar) in the `@brief` that documents the function associated with prize_route_t
(the helper that returns shared memory size for a route of given size), so the
comment matches the prize_route_t API and avoids confusion.
In `@cpp/src/routing/route/service_time_route.cuh`:
- Line 143: Update the documentation string so the `@brief` describes sizing
shared memory for service-time data rather than a "cost route": change the
phrase to reference service_time_route_t (or “service-time route”) and indicate
it returns the shared memory size required to store service-time data for a
route of a given size; update the comment above the function that computes
shared memory for service_time_route_t accordingly.
In `@cpp/src/routing/route/tasks_route.cuh`:
- Line 138: The doc comment for the shared-memory-size helper incorrectly calls
it a "cost route"; update the `@brief` to refer to tasks-route storage instead.
Locate the helper function that computes shared memory size for tasks_route_t
(reference symbol tasks_route_t and the shared-size helper function near its
comment) and replace "cost route" with "tasks-route" (or "tasks-route storage")
so the brief accurately describes that it returns the shared memory size
required to store a tasks_route_t of a given size.
In `@cpp/src/routing/route/vehicle_fixed_cost_route.cuh`:
- Line 107: Update the doc comment's `@brief` to explicitly reference "vehicle
fixed cost route" instead of the generic "cost route": replace "Get the shared
memory size required to store a cost route of a given size" with "Get the shared
memory size required to store a vehicle fixed cost route of a given size" in the
header comment above the vehicle_fixed_cost_route declarations so the API docs
clearly target the vehicle-fixed-cost route data.
In `@cpp/src/routing/util_kernels/set_nodes_data.cuh`:
- Around line 57-58: Add a unit/regression test that verifies COST boundary
initialization after route setup: exercise the route setup path that
constructs/populates a route, then assert route.template
get_dim<dim_t::COST>().cost_backward[n_nodes_route] == 0.f and route.template
get_dim<dim_t::COST>().cost_forward[0] == 0.f; place the test alongside existing
routing unit tests (matching the project's test patterns for .cpp/.cc/.cxx) and
name it to reflect "COST boundary initialization" so future changes to
get_dim<dim_t::COST>(), cost_backward, cost_forward or route initialization will
fail the test if regressions occur.
---
Nitpick comments:
In `@cpp/src/routing/ges/lexicographic_search/lexicographic_search.cu`:
- Around line 391-396: Rename the local boolean is_delivery_time_dist_feasible
to match the naming used elsewhere (e.g., is_delivery_time_and_dist_feasible)
for consistency; update its declaration and all uses in the block where
node_stack.delivery_node.time_dim.forward_feasible(...) and
node_stack.delivery_node.cost_dim.forward_feasible(...) are checked (symbols:
node_stack, delivery_node, time_dim, cost_dim, forward_feasible, s_route,
vehicle_info) so the new name replaces the old one throughout that conditional.
- Around line 311-316: The variable name is_delivery_time_dist_feasible is
misleading because it now checks delivery_node.cost_dim rather than distance;
rename it to is_delivery_time_cost_feasible wherever declared and referenced
(the boolean assigned from
node_stack.delivery_node.time_dim.forward_feasible(node_stack.s_route.vehicle_info())
&&
node_stack.delivery_node.cost_dim.forward_feasible(node_stack.s_route.vehicle_info())
and the subsequent if (!...) check) so the identifier matches the checked
dimensions (refer to node_stack, delivery_node, time_dim, cost_dim, and
s_route.vehicle_info()) and update all usages accordingly.
In `@cpp/src/routing/ges/lexicographic_search/node_stack.cuh`:
- Around line 213-218: The local variable shared_for_dist_buffers is misnamed
because it actually sizes buffers for the cost dimension; rename it to something
clearer like shared_for_cost_buffers (or shared_for_cost_dim_buffers) where it's
declared and used in the block guarded by
solution_ptr->problem_ptr->dimensions_info.cost_dim.has_constraints(), and
update any references (e.g., the sh_size += shared_for_dist_buffers line) to the
new name so the intent (cost-dimension buffer sizing) is clear.
In `@cpp/src/routing/local_search/vrp/vrp_search.cu`:
- Around line 22-40: Rename the kernel compute_reverse_distances to
compute_reverse_costs to match the fact it computes cost_dim.reverse_cost using
dim_t::COST; update all references and declarations (e.g., the kernel definition
template<typename i_t, typename f_t, request_t REQUEST> __global__ void
compute_reverse_distances(typename solution_t<i_t, f_t, REQUEST>::view_t
solution) and any kernel launches) and ensure the symbol name change is
reflected where route.dimensions.cost_dim.reverse_cost and
get_arc_of_dimension<i_t,f_t,dim_t::COST> are used so callers and tests compile
against compute_reverse_costs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bbf9b4f5-5897-4bb3-90ca-5f578d66d74e
📒 Files selected for processing (26)
cpp/include/cuopt/routing/data_model_view.hppcpp/src/routing/arc_value.hppcpp/src/routing/dimensions.cuhcpp/src/routing/diversity/helpers.hppcpp/src/routing/diversity/macros.hppcpp/src/routing/ges/lexicographic_search/lexicographic_search.cucpp/src/routing/ges/lexicographic_search/node_stack.cuhcpp/src/routing/local_search/compute_compatible.cucpp/src/routing/local_search/permutation_helper.cuhcpp/src/routing/local_search/sliding_tsp.cucpp/src/routing/local_search/sliding_window.cucpp/src/routing/local_search/two_opt.cucpp/src/routing/local_search/vrp/fragment_kernels.cuhcpp/src/routing/local_search/vrp/vrp_search.cucpp/src/routing/node/cost_node.cuhcpp/src/routing/node/node.cuhcpp/src/routing/problem/problem.cucpp/src/routing/route/cost_route.cuhcpp/src/routing/route/dimensions_route.cuhcpp/src/routing/route/distance_route.cuhcpp/src/routing/route/mismatch_route.cuhcpp/src/routing/route/prize_route.cuhcpp/src/routing/route/service_time_route.cuhcpp/src/routing/route/tasks_route.cuhcpp/src/routing/route/vehicle_fixed_cost_route.cuhcpp/src/routing/util_kernels/set_nodes_data.cuh
💤 Files with no reviewable changes (1)
- cpp/src/routing/route/distance_route.cuh
rg20
reviewed
May 26, 2026
Contributor
rg20
left a comment
rg20
left a comment
There was a problem hiding this comment.
Thanks for the PR.
I have minor comments, these already caught by coderabbit
| @@ -265,7 +265,7 @@ class dimensions_route_t { | |||
| request_route_t<i_t, f_t, REQUEST> requests; | |||
|
|
|||
| // distance route | |||
Contributor
There was a problem hiding this comment.
Suggested change
| // distance route | |
| // cost route |
There was a problem hiding this comment.
Thank you @rg20 for pointing this out. This modification was not correctly applied during the standardization process carried out in this PR. Issue resolved in commit:
refactor(routing): update distance reference comments in routing files.
|
|
||
| /** | ||
| * @brief Get the shared memory size required to store a distance route of a given size | ||
| * @brief Get the shared memory size required to store a cost route of a given size |
Contributor
There was a problem hiding this comment.
Suggested change
| * @brief Get the shared memory size required to store a cost route of a given size | |
| * @brief Get the shared memory size required to store a mismatch route of a given size |
There was a problem hiding this comment.
As done in the previous issue, this one has been also resolved in commit:
refactor(routing): update distance reference comments in routing files.
|
|
||
| /** | ||
| * @brief Get the shared memory size required to store a distance route of a given size | ||
| * @brief Get the shared memory size required to store a cost route of a given size |
Contributor
There was a problem hiding this comment.
Suggested change
| * @brief Get the shared memory size required to store a cost route of a given size | |
| * @brief Get the shared memory size required to store a prize route of a given size |
There was a problem hiding this comment.
Resolved in commit:
refactor(routing): update distance reference comments in routing files.
|
|
||
| /** | ||
| * @brief Get the shared memory size required to store a distance route of a given size | ||
| * @brief Get the shared memory size required to store a cost route of a given size |
Contributor
There was a problem hiding this comment.
Suggested change
| * @brief Get the shared memory size required to store a cost route of a given size | |
| * @brief Get the shared memory size required to store a service time route of a given size |
There was a problem hiding this comment.
Resolved in commit:
refactor(routing): update distance reference comments in routing files.
|
|
||
| /** | ||
| * @brief Get the shared memory size required to store a distance route of a given size | ||
| * @brief Get the shared memory size required to store a cost route of a given size |
Contributor
There was a problem hiding this comment.
Suggested change
| * @brief Get the shared memory size required to store a cost route of a given size | |
| * @brief Get the shared memory size required to store a tasks route of a given size |
There was a problem hiding this comment.
Resolved in commit:
refactor(routing): update distance reference comments in routing files.
|
|
||
| /** | ||
| * @brief Get the shared memory size required to store a distance route of a given size | ||
| * @brief Get the shared memory size required to store a cost route of a given size |
Contributor
There was a problem hiding this comment.
Suggested change
| * @brief Get the shared memory size required to store a cost route of a given size | |
| * @brief Get the shared memory size required to store a vehicle fixed cost route of a given size |
There was a problem hiding this comment.
Resolved in commit:
refactor(routing): update distance reference comments in routing files.
rg20
added
improvement
Modify comments related to distance references in dimensions, route, prize, service, tasks, and vehicle fixed files under /cpp/routing. Signed-off-by: Juanfran-Robles <juanfrancisco.robles@oga.ai>
Cover VRP and PDP route setup by dirtying COST boundary buffers and verifying set_route_data resets cost_forward[0] and cost_backward[n] Signed-off-by: Juanfran-Robles <juanfrancisco.robles@oga.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary
This PR standardizes naming across the routing package by replacing
distanceterminology withcostterminology wherever the valuerepresents the generic cuOpt cost matrix rather than a physical distance.
The change improves semantic consistency across routing internals and
aligns the implementation with the existing
data_model_view.hppdocumentation, which already describes the cost matrix as supporting
arbitrary user-defined metrics (e.g. time, miles, meters, euros, or
other real-valued costs).
All changes are limited to the
routingpackage and do not modifyoptimization behavior.
Changes Made
Renamed
distance-related routing helpers, variables, and types tocost-based equivalents where appropriate.Replaced:
distance_dim→cost_dimdistance_node_t→cost_node_tdistance_route_t→cost_route_tRenamed:
distance_node.cuh→cost_node.cuhdistance_route.cuh→cost_route.cuhUpdated all dependent routing internals and local-search consumers.
Adjusted wording in
data_model_view.hppto consistently refer togeneric optimization costs rather than physical distance.
Scope
Affected components include:
Impact
Validation
Notes
This change touches internal routing identifiers and file names.
Downstream branches using the old
distance_*internal symbols mayrequire rebasing or reference updates.
Issue
N/A
Checklist