Fix vehicle fixed cost accounting in fragment-vs-route deltas - #1244

mlubin · 2026-05-19T18:12:51Z

(Claude-discovered fix.)

The cycle-finder's total_cycle_cost was systematically under-counting vehicle fixed cost when a cycle activated a previously-empty route via the SPECIAL (unrouted) pseudo-node. The realized post-move cost included the activation, the predicted total didn't — manifesting as flaky cost_after - cost_before - total_cycle_cost < 1. aborts in level0_retail/retail_float_test_t.CVRPTW_Retail/18.

Root cause: vehicle_fixed_cost_node_t::get_cost was a no-op. When node_t::calculate_forward_all_and_delta aggregated the fragment's per-dimension costs, the VEHICLE_FIXED_COST dimension contributed 0 from the new (fragment) side while old_obj_cost carried the route's actual fixed cost. A defensive obj_weights[VEHICLE_FIXED_COST] = 0 line stripped both sides symmetrically, which kept same-route same- vehicle moves correct but silently dropped the activation cost any time a cycle changed a route's occupancy.

Fix: have the fragment-side get_cost contribute vehicle_info.fixed_cost so the fragment correctly represents an active route. With the fragment correct, several caller-side workarounds become redundant:

node.cuh: drop the obj_weights[VEHICLE_FIXED_COST] = 0 zero-out in both forward and backward delta paths.
vrp_search.cu::evaluate_2_opt_route: drop the manual (F_new − F_old) add for HVRP vehicle swaps; the fragment cost now carries the swap delta directly.
ox_kernels.cuh and ox_recombiner.cuh: drop the manual optimal_vehicle_fixed_cost + in OX edge weights; cost_combine now returns the full edge cost.
local_search.cu: re-enable the Cost mismatch after a move assertion at the strict < 1. bound.

Verified by running CVRPTW_Retail/18 for 300 iterations under ASSERT_MODE: 0 aborts and 0 near-misses (residual >= 0.1), versus a pre-fix ~1.7% per-iteration abort rate. Full ROUTING_TEST suite passes.

Closes #845

copy-pr-bot · 2026-05-19T18:12:55Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

akifcorduk

I think it looks good, thanks Miles! I would also ask Rajesh's reviews on possible side effects as he had some comments on the previous cost computation on node.cuh

coderabbitai · 2026-05-19T18:32:36Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e4fb66d4-8e38-4fb4-9f96-8cc62be5daa6

📥 Commits

Reviewing files that changed from the base of the PR and between 189099c and 8d6305e.

📒 Files selected for processing (4)

cpp/src/routing/crossovers/ox_kernels.cuh
cpp/src/routing/local_search/vrp/vrp_search.cu
cpp/src/routing/node/node.cuh
cpp/src/routing/node/vehicle_fixed_cost_node.cuh

🚧 Files skipped from review as they are similar to previous changes (3)

cpp/src/routing/local_search/vrp/vrp_search.cu
cpp/src/routing/node/vehicle_fixed_cost_node.cuh
cpp/src/routing/crossovers/ox_kernels.cuh

📝 Walkthrough

Walkthrough

Centralizes vehicle fixed-cost injection into vehicle_fixed_cost_node_t::get_cost, removes explicit fixed-cost special-casing from delta and edge computations, and adds a runtime assertion validating cycle-cost consistency after local-search moves.

Changes

Vehicle Fixed Cost Centralization

Layer / File(s)	Summary
Vehicle fixed cost getter implementation `cpp/src/routing/node/vehicle_fixed_cost_node.cuh`, `cpp/src/routing/node/node.cuh`	Implements `vehicle_fixed_cost_node_t::get_cost` to set `objective_t::VEHICLE_FIXED_COST` from `vehicle_info.fixed_cost` and updates SPDX headers.
Node objective delta calculations `cpp/src/routing/node/node.cuh`	Forward and backward delta helpers use the full `dimensions_info.objective_weights` dot product with `(new_obj_cost - old_obj_cost)` (removed zeroing of VEHICLE_FIXED_COST).
Local search move validation and VRP 2-opt evaluation `cpp/src/routing/local_search/local_search.cu`, `cpp/src/routing/local_search/vrp/vrp_search.cu`	After applying negative-cycle moves, recompute solution cost and assert (cost_after - cost_before) ≈ `move_candidates.cycles.total_cycle_cost`; stop adding explicit vehicle fixed-cost correction in 2-opt evaluation and update SPDX header.
Crossover edge cost calculations `cpp/src/routing/crossovers/ox_kernels.cuh`, `cpp/src/routing/crossovers/ox_recombiner.cuh`	Write edge weights using `cost` directly (removed adding `optimal_vehicle_fixed_cost`), relying on `vehicle_fixed_cost_node_t::get_cost` to include fixed cost; update SPDX header.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing vehicle fixed cost accounting in fragment-vs-route delta calculations, which is the core objective across all modified files.
Description check	✅ Passed	The description clearly explains the root cause (vehicle_fixed_cost_node_t::get_cost was a no-op), the fix strategy (make fragment-side get_cost return vehicle_info.fixed_cost), and the resulting caller-side cleanups, with verification results.
Linked Issues check	✅ Passed	The PR directly addresses issue `#845`, which reported an assertion failure due to cost mismatch. The changes ensure fragments correctly represent active route costs, eliminating the under-counting bug that caused the assertion.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing vehicle fixed cost accounting: core fix in vehicle_fixed_cost_node.cuh, removal of workarounds in node.cuh/vrp_search.cu/ox_kernels.cuh/ox_recombiner.cuh/local_search.cu, and copyright year updates. No unrelated changes detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 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/local_search/local_search.cu`:
- Around line 314-315: The assertion only checks an upper bound and misses large
negative mismatches; update the consistency check in the local search to use an
absolute difference: replace the current cuopt_assert condition that compares
(cost_after - cost_before) - move_candidates.cycles.total_cycle_cost < 1. with
one that tests the absolute error, e.g. fabs((cost_after - cost_before) -
move_candidates.cycles.total_cycle_cost) < 1. so the assertion is symmetric;
ensure any needed math header/functions are available for fabs (or std::abs)
where cuopt_assert is invoked.

🪄 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: 7fd82fc6-4658-464e-8ac4-16cc1c1eb806

📥 Commits

Reviewing files that changed from the base of the PR and between b145cc3 and b4ff2d9.

📒 Files selected for processing (6)

cpp/src/routing/crossovers/ox_kernels.cuh
cpp/src/routing/crossovers/ox_recombiner.cuh
cpp/src/routing/local_search/local_search.cu
cpp/src/routing/local_search/vrp/vrp_search.cu
cpp/src/routing/node/node.cuh
cpp/src/routing/node/vehicle_fixed_cost_node.cuh

coderabbitai · 2026-05-19T18:32:40Z

+      cuopt_assert((cost_after - cost_before) - move_candidates.cycles.total_cycle_cost < 1.,
+                   "Cost mismatch after a move");


⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Make the cycle-cost consistency assert symmetric.

Line 314 currently only checks an upper bound; large negative mismatches still pass. Use absolute error for a true consistency check.

Suggested fix

- cuopt_assert((cost_after - cost_before) - move_candidates.cycles.total_cycle_cost < 1., + cuopt_assert(abs((cost_after - cost_before) - move_candidates.cycles.total_cycle_cost) < 1., "Cost mismatch after a move");

🤖 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/local_search.cu` around lines 314 - 315, The assertion only checks an upper bound and misses large negative mismatches; update the consistency check in the local search to use an absolute difference: replace the current cuopt_assert condition that compares (cost_after - cost_before) - move_candidates.cycles.total_cycle_cost < 1. with one that tests the absolute error, e.g. fabs((cost_after - cost_before) - move_candidates.cycles.total_cycle_cost) < 1. so the assertion is symmetric; ensure any needed math header/functions are available for fabs (or std::abs) where cuopt_assert is invoked.

hlinsen

Looks good, thanks @mlubin!

mlubin · 2026-05-19T19:02:28Z

@hlinsen @akifcorduk do you like claude's in-line comments or should they be deleted?

akifcorduk · 2026-05-19T19:05:22Z

@mlubin I think they can stay, they are explaining some non-trivial logic.

hlinsen · 2026-05-19T19:06:00Z

@hlinsen @akifcorduk do you like claude's in-line comments or should they be deleted?

They're useful we can keep them

coderabbitai

🧹 Nitpick comments (1)

cpp/src/routing/crossovers/ox_recombiner.cuh (1)

844-845: ⚡ Quick win

Remove unused variable optimal_vehicle_fixed_cost.

The optimal_vehicle_fixed_cost variable is computed but no longer used after Line 872 was updated to use cost directly (which now includes the fixed cost via vehicle_fixed_cost_node_t::get_cost).

♻️ Proposed fix to remove dead code

-    double optimal_vehicle_fixed_cost =
-      dimensions_info.has_dimension(dim_t::VEHICLE_FIXED_COST) ? vehicle_info.fixed_cost : 0.;
-
     // Here we have to use the vehicle type 'veh'

🤖 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/crossovers/ox_recombiner.cuh` around lines 844 - 845, Remove
the unused local variable optimal_vehicle_fixed_cost (and its conditional
assignment using dimensions_info.has_dimension(dim_t::VEHICLE_FIXED_COST) ?
vehicle_info.fixed_cost : 0.) from ox_recombiner.cuh; the code now uses cost
(which already includes the fixed cost via vehicle_fixed_cost_node_t::get_cost),
so delete the optimal_vehicle_fixed_cost declaration and assignment to eliminate
dead code and any compiler warnings.

🤖 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.

Nitpick comments:
In `@cpp/src/routing/crossovers/ox_recombiner.cuh`:
- Around line 844-845: Remove the unused local variable
optimal_vehicle_fixed_cost (and its conditional assignment using
dimensions_info.has_dimension(dim_t::VEHICLE_FIXED_COST) ?
vehicle_info.fixed_cost : 0.) from ox_recombiner.cuh; the code now uses cost
(which already includes the fixed cost via vehicle_fixed_cost_node_t::get_cost),
so delete the optimal_vehicle_fixed_cost declaration and assignment to eliminate
dead code and any compiler warnings.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5b614d74-ae86-4bd2-9187-7d41ff3b85b1

📥 Commits

Reviewing files that changed from the base of the PR and between b4ff2d9 and 3dbc941.

📒 Files selected for processing (6)

cpp/src/routing/crossovers/ox_kernels.cuh
cpp/src/routing/crossovers/ox_recombiner.cuh
cpp/src/routing/local_search/local_search.cu
cpp/src/routing/local_search/vrp/vrp_search.cu
cpp/src/routing/node/node.cuh
cpp/src/routing/node/vehicle_fixed_cost_node.cuh

🚧 Files skipped from review as they are similar to previous changes (4)

cpp/src/routing/crossovers/ox_kernels.cuh
cpp/src/routing/node/node.cuh
cpp/src/routing/local_search/local_search.cu
cpp/src/routing/local_search/vrp/vrp_search.cu

The cycle-finder's `total_cycle_cost` was systematically under-counting vehicle fixed cost when a cycle activated a previously-empty route via the SPECIAL (unrouted) pseudo-node. The realized post-move cost included the activation, the predicted total didn't — manifesting as flaky `cost_after - cost_before - total_cycle_cost < 1.` aborts in `level0_retail/retail_float_test_t.CVRPTW_Retail/18`. Root cause: `vehicle_fixed_cost_node_t::get_cost` was a no-op. When `node_t::calculate_forward_all_and_delta` aggregated the fragment's per-dimension costs, the VEHICLE_FIXED_COST dimension contributed 0 from the new (fragment) side while `old_obj_cost` carried the route's actual fixed cost. A defensive `obj_weights[VEHICLE_FIXED_COST] = 0` line stripped both sides symmetrically, which kept same-route same- vehicle moves correct but silently dropped the activation cost any time a cycle changed a route's occupancy. Fix: have the fragment-side `get_cost` contribute `vehicle_info.fixed_cost` so the fragment correctly represents an active route. With the fragment correct, several caller-side workarounds become redundant: - `node.cuh`: drop the `obj_weights[VEHICLE_FIXED_COST] = 0` zero-out in both forward and backward delta paths. - `vrp_search.cu::evaluate_2_opt_route`: drop the manual `(F_new − F_old)` add for HVRP vehicle swaps; the fragment cost now carries the swap delta directly. - `ox_kernels.cuh` and `ox_recombiner.cuh`: drop the manual `optimal_vehicle_fixed_cost +` in OX edge weights; `cost_combine` now returns the full edge cost. - `local_search.cu`: re-enable the `Cost mismatch after a move` assertion at the strict `< 1.` bound. Verified by hammering CVRPTW_Retail/18 for 300 iterations under ASSERT_MODE: 0 aborts and 0 near-misses (residual >= 0.1), versus a pre-fix ~1.7% per-iteration abort rate. Full ROUTING_TEST suite passes. Closes NVIDIA#845 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Miles Lubin <mlubin@nvidia.com>

mlubin · 2026-05-19T20:32:53Z

/ok to test 189099c

copy-pr-bot · 2026-05-19T20:32:56Z

/ok to test 189099c

@mlubin, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

mlubin · 2026-05-19T20:33:22Z

/ok to test 180458f

Pre-commit fixes: - node.cuh: reflow two `objective_cost_t::dot` calls per clang-format - bump SPDX copyright year on the four files touched by the previous commit Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Miles Lubin <mlubin@nvidia.com>

mlubin · 2026-05-19T20:45:05Z

/ok to test 8d6305e

mlubin · 2026-05-20T02:05:19Z

I'll merge given the two approvals and rare green checkmark. @rg20 ptal at your convenience and I'll follow up on any comments.

mlubin · 2026-05-20T02:05:23Z

/merge

mlubin added bug Something isn't working non-breaking Introduces a non-breaking change labels May 19, 2026

akifcorduk approved these changes May 19, 2026

View reviewed changes

mlubin requested a review from rg20 May 19, 2026 18:29

mlubin marked this pull request as ready for review May 19, 2026 18:29

mlubin requested a review from a team as a code owner May 19, 2026 18:29

mlubin requested review from Bubullzz and removed request for Bubullzz May 19, 2026 18:29

coderabbitai Bot reviewed May 19, 2026

View reviewed changes

hlinsen approved these changes May 19, 2026

View reviewed changes

mlubin force-pushed the fix/cycle-finder-vehicle-fixed-cost branch from b4ff2d9 to 3dbc941 Compare May 19, 2026 19:09

coderabbitai Bot reviewed May 19, 2026

View reviewed changes

mlubin force-pushed the fix/cycle-finder-vehicle-fixed-cost branch from 3dbc941 to 189099c Compare May 19, 2026 19:20

Merge branch 'main' into fix/cycle-finder-vehicle-fixed-cost

180458f

rapids-bot Bot merged commit 785f148 into NVIDIA:main May 20, 2026
185 of 191 checks passed

mlubin deleted the fix/cycle-finder-vehicle-fixed-cost branch May 20, 2026 02:05

		cuopt_assert((cost_after - cost_before) - move_candidates.cycles.total_cycle_cost < 1.,
		"Cost mismatch after a move");

Uh oh!

Conversation

mlubin commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

copy-pr-bot Bot commented May 19, 2026

Uh oh!

akifcorduk left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot May 19, 2026

Choose a reason for hiding this comment

Uh oh!

hlinsen left a comment

Choose a reason for hiding this comment

Uh oh!

mlubin commented May 19, 2026

Uh oh!

akifcorduk commented May 19, 2026

Uh oh!

hlinsen commented May 19, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

mlubin commented May 19, 2026

Uh oh!

copy-pr-bot Bot commented May 19, 2026

Uh oh!

mlubin commented May 19, 2026

Uh oh!

mlubin commented May 19, 2026

Uh oh!

mlubin commented May 20, 2026

Uh oh!

mlubin commented May 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mlubin commented May 19, 2026 •

edited

Loading

coderabbitai Bot commented May 19, 2026 •

edited

Loading