Skip to content

Skip optionally the cost matrix validation check #1217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rapids-bot merged 3 commits into from
May 15, 2026
+9 −2
Merged

Skip optionally the cost matrix validation check #1217

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8cac3fb
Skip matrix validation check
hlinsen May 14, 2026
945ac73
Fix style
hlinsen May 14, 2026
333ffd2
Merge branch 'main' of github.com:NVIDIA/cuopt into matrix-validation
hlinsen May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions python/cuopt/cuopt/routing/vehicle_routing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ def __init__(self, n_locations, n_fleet, n_orders: int = -1):
super().__init__(n_locations, n_fleet, n_orders=n_orders)

@catch_cuopt_exception
def add_cost_matrix(self, cost_mat, vehicle_type=0):
def add_cost_matrix(
self, cost_mat, vehicle_type=0, *, skip_validation=False
):
"""
Add a matrix for all locations (vehicle/technician locations included)
at once.
Expand All @@ -84,6 +86,10 @@ def add_cost_matrix(self, cost_mat, vehicle_type=0):
num_location rows and columns.
vehicle_type : uint8
Identifier of the vehicle.
skip_validation : bool
If True, skips Python validation for matrix shape, NULL values,
and non-negative values. The caller is responsible for providing
a valid square matrix matching the number of locations.

Examples
--------
Expand Down Expand Up @@ -125,7 +131,8 @@ def add_cost_matrix(self, cost_mat, vehicle_type=0):
if vehicle_type in self.costs:
raise ValueError("Vehicle type matrix has already been added")

validate_matrix(cost_mat, "cost matrix", self.get_num_locations())
if not skip_validation:
validate_matrix(cost_mat, "cost matrix", self.get_num_locations())
Comment on lines +134 to +135
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Retain minimal boundary validation when skip_validation=True.

Line 132 currently skips all checks, so malformed matrix dimensions can cross the Python boundary and fail deeper in the wrapper with less actionable errors. Keep a cheap shape guard in the skip path and reserve full scans (NULL/non-negative checks) for the non-skip path.

Suggested patch 
-        if not skip_validation:
-            validate_matrix(cost_mat, "cost matrix", self.get_num_locations())
+        n_locations = self.get_num_locations()
+        if skip_validation:
+            if (
+                len(cost_mat) != n_locations
+                or len(cost_mat.columns) != n_locations
+            ):
+                raise ValueError(
+                    "cost matrix must be square with size equal to "
+                    "number of locations"
+                )
+        else:
+            validate_matrix(cost_mat, "cost matrix", n_locations)

As per coding guidelines, "Flag missing input validation at library and server boundaries."

🤖 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 `@python/cuopt/cuopt/routing/vehicle_routing.py` around lines 132 - 133, When
skip_validation is True you still need a lightweight shape guard to avoid
malformed matrices crossing into the wrapper: add a cheap check before returning
that cost_mat is a 2D sequence with outer length == n and every row length == n
(where n = self.get_num_locations()), but do not perform the heavier
NULL/non-negative content scans that validate_matrix does; update the branch
handling skip_validation around the validate_matrix call to run this simple
shape guard (using cost_mat and self.get_num_locations() as identifiers) and
only call validate_matrix when skip_validation is False.


super().add_cost_matrix(cost_mat, vehicle_type)
Comment on lines +134 to 137
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add pytest coverage for both skip_validation paths.

This change adds a new behavior branch but no tests in this PR context. Please add tests for default validation behavior and explicit skip behavior (while still validating duplicate vehicle_type handling).

As per coding guidelines, "python/cuopt/cuopt/**/*.py: Add unit tests for Python code using pytest, referencing examples in python/cuopt/cuopt/tests".

🤖 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 `@python/cuopt/cuopt/routing/vehicle_routing.py` around lines 134 - 137, Add
pytest unit tests covering both branches introduced around skip_validation:
write one test that calls vehicle_routing.add_cost_matrix(...) with
skip_validation=False (or default) to assert validate_matrix is invoked and that
a bad cost_mat raises the expected validation exception, and a second test that
calls add_cost_matrix(..., skip_validation=True) to ensure validate_matrix is
not called but duplicate vehicle_type handling still triggers the same
error/behavior; reference the functions/methods validate_matrix,
add_cost_matrix, get_num_locations and the vehicle_type parameter to locate the
logic and include assertions for duplicate vehicle_type behavior when adding the
same vehicle_type twice.


Expand Down
Loading