Use fp64 for all node stack buffers and fix indexing in vehicle order match test

[hlinsen]

Fixes:

• [BUG] Assertion failure on the CVRPTW test #1077
• [BUG] CI flaky routing vehicle order match test #1132

[coderabbitai]

📝 Walkthrough

Walkthrough

Added stricter input validation in vehicle/order match methods, changed forward-feasibility buffer storage to double in the lexicographic node stack, and adjusted a test's constraint-removal sampling to compute vehicle index from id / this->n_locations.

Changes

Cohort / File(s)	Summary
Input validation cpp/src/routing/data_model_view.cu	Added cuopt_expects checks in add_vehicle_order_match and add_order_vehicle_match to validate provided indices, positive counts, and that device arrays contain IDs within valid ranges via detail::check_min_max_values.
Forward-feasibility storage cpp/src/routing/ges/lexicographic_search/node_stack.cuh	Changed node_stack_t::item_t members departure_forward and excess_forward from template type f_t to double, affecting copying and runtime comparisons of forward time buffers.
Test sampling logic cpp/tests/routing/level0/l0_vehicle_order_match.cu	Changed test sampling mapping so vehicle is computed from id / this->n_locations instead of id / this->n_vehicles, altering which pairs are removed prior to building vehicle_order_match_d.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 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 summarizes the two main changes: using fp64 for node stack buffers and fixing indexing in the vehicle order match test, matching the changeset content.
Description check	✅ Passed	The description references the two issues being fixed, which directly relate to the changes in the changeset addressing CVRPTW assertions and vehicle order test flakiness.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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]

🧹 Nitpick comments (2)

cpp/src/routing/data_model_view.cu (2)

302-304: Please confirm intended behavior for empty match lists.

Line 302 and Line 322 now reject zero-length compatibility lists (norders > 0, nvehicles > 0). If callers previously used empty lists to represent “no allowed matches,” this is a contract change. Please verify intent and, if intentional, update API docs accordingly.

Also applies to: 322-324

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cpp/src/routing/data_model_view.cu` around lines 302 - 304, The change
rejects zero-length compatibility lists by asserting norders > 0 and nvehicles >
0 via cuopt_expects (the checks that emit "number of orders in
vehicle_order_match must be positive" and similar for vehicles); confirm whether
empty lists should mean “no allowed matches” or are invalid—if empty lists
should be allowed, remove or relax these cuopt_expects checks in the
vehicle_order_match validation code (and any other places using
norders/nvehicles checks) to accept zero and handle it downstream; if the change
is intentional, update the public API docs and function comments for
vehicle_order_match (and the corresponding vehicle validation) to state that
lists must be non-empty and include the rationale and examples.

---

307-310: Consider making deep range validation optional for bulk setup paths.

These checks call detail::check_min_max_values, which synchronizes the stream internally; repeated calls per vehicle/order can add avoidable setup latency on large instances. A validate_input toggle (or batched validation) would preserve safety while reducing sync overhead when inputs are prevalidated.

As per coding guidelines: "Eliminate unnecessary host-device synchronization (cudaDeviceSynchronize) in hot paths that blocks GPU pipeline; use streams and events for async execution."

Also applies to: 327-330

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cpp/src/routing/data_model_view.cu` around lines 307 - 310, The range-check
calls using cuopt_expects(detail::check_min_max_values(...), ...) cause
host-device syncs; add an optional boolean parameter (e.g., validate_input =
true) to the API/function that contains these calls so callers can skip deep
validation for bulk/prevalidated inputs, and wrap the cuopt_expects calls for
orders and vehicles with if (validate_input) { ... } so check_min_max_values is
only invoked when requested (preserve existing behavior by default). Use the
same pattern for the other occurrence that currently calls
detail::check_min_max_values with handle_ptr_->get_stream() to avoid repeated
stream synchronizations in hot setup paths. Ensure the function signature and
any calling sites propagate the new flag or provide an overload/default so
behavior is opt-in and tests still exercise the validation path.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@cpp/src/routing/data_model_view.cu`:
- Around line 302-304: The change rejects zero-length compatibility lists by
asserting norders > 0 and nvehicles > 0 via cuopt_expects (the checks that emit
"number of orders in vehicle_order_match must be positive" and similar for
vehicles); confirm whether empty lists should mean “no allowed matches” or are
invalid—if empty lists should be allowed, remove or relax these cuopt_expects
checks in the vehicle_order_match validation code (and any other places using
norders/nvehicles checks) to accept zero and handle it downstream; if the change
is intentional, update the public API docs and function comments for
vehicle_order_match (and the corresponding vehicle validation) to state that
lists must be non-empty and include the rationale and examples.
- Around line 307-310: The range-check calls using
cuopt_expects(detail::check_min_max_values(...), ...) cause host-device syncs;
add an optional boolean parameter (e.g., validate_input = true) to the
API/function that contains these calls so callers can skip deep validation for
bulk/prevalidated inputs, and wrap the cuopt_expects calls for orders and
vehicles with if (validate_input) { ... } so check_min_max_values is only
invoked when requested (preserve existing behavior by default). Use the same
pattern for the other occurrence that currently calls
detail::check_min_max_values with handle_ptr_->get_stream() to avoid repeated
stream synchronizations in hot setup paths. Ensure the function signature and
any calling sites propagate the new flag or provide an overload/default so
behavior is opt-in and tests still exercise the validation path.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 865cbcf1-bfd9-4eb2-9f21-4ceeee4138f8

📥 Commits

Reviewing files that changed from the base of the PR and between 79fd1c8 and 1a19fe9.

📒 Files selected for processing (2)

• cpp/src/routing/data_model_view.cu
• cpp/src/routing/ges/lexicographic_search/node_stack.cuh

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

• cpp/src/routing/ges/lexicographic_search/node_stack.cuh