refactor(routing): standardize cost terminology - #1299
refactor(routing): standardize cost terminology#1299Juan-Francisco-Robles wants to merge 29 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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (28)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (26)
📝 WalkthroughWalkthroughRefactors routing to use a COST primary dimension across enums, dispatch, node and route storage, local-search calculations, shared-memory buffers, initialization, documentation, and CUDA tests. ChangesDimension System and Local Search Refactoring
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
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 | |||
There was a problem hiding this comment.
| // 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 |
There was a problem hiding this comment.
| * @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 |
There was a problem hiding this comment.
| * @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 |
There was a problem hiding this comment.
| * @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 |
There was a problem hiding this comment.
| * @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 |
There was a problem hiding this comment.
| * @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>
|
All review comments have been addressed. |
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
|
/ok to test ac4e9b4 |
@josembd please see the error from check_style below. If you run the pre-commit hooks to fix the style errors, then the cpp build will run and we can get full test results.If you are seeing this message in CI, reproduce locally with: |
CI Test Summary✅ All 31 test job(s) passed. |
Signed-off-by: Jose Maria Baca <josemaria.baca@oga.ai>
|
/ok to test 9f30942 |
|
🔔 Hi @anandhkb @rg20, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
|
Hi @rg20, thanks for reviewing and approving the PR. We noticed that this PR is still open in the 26.08 milestone, which is due today. Could you let us know whether anything else is needed from our side before it can be merged, such as updating the branch with the latest changes, rerunning the checks, or retargeting the PR to the We also have another contribution that introduces a separate feature, but its implementation depends on the changes introduced in this PR. More generally, for this and future contributions with technical dependencies between them, what workflow do you prefer? Should we wait until the prerequisite PR is merged before opening the dependent one, or is it acceptable to open it as a stacked PR in the meantime? Since these are among our first contributions to cuOpt, we want to make sure we are following the workflow that is most convenient for the maintainers. Thanks for your guidance. |
Since the release is today, we normally don't merge any new PRs since there are chances of breakages. So I will move this to 26.10 version. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
🔔 Hi @anandhkb @rg20, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
1 similar comment
|
🔔 Hi @anandhkb @rg20, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
6 participants
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