Skip optionally the cost matrix validation check#1217
Merged
Conversation
📝 WalkthroughWalkthroughThe ChangesSkip validation parameter for cost matrix
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 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: 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 `@python/cuopt/cuopt/routing/vehicle_routing.py`:
- Around line 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.
🪄 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: c0284935-6266-45f0-97fc-8eb2c739dc8b
📒 Files selected for processing (1)
python/cuopt/cuopt/routing/vehicle_routing.py
Comment on lines
+132
to
+133
| if not skip_validation: | ||
| validate_matrix(cost_mat, "cost matrix", self.get_num_locations()) |
There was a problem hiding this comment.
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.
rgsl888prabhu
approved these changes
May 14, 2026
rg20
approved these changes
May 14, 2026
There was a problem hiding this comment.
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 `@python/cuopt/cuopt/routing/vehicle_routing.py`:
- Around line 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.
🪄 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: a273be76-3bcf-436a-afc8-8cd30cfd5ef0
📒 Files selected for processing (1)
python/cuopt/cuopt/routing/vehicle_routing.py
Comment on lines
+134
to
137
| if not skip_validation: | ||
| validate_matrix(cost_mat, "cost matrix", self.get_num_locations()) | ||
|
|
||
| super().add_cost_matrix(cost_mat, vehicle_type) |
There was a problem hiding this comment.
🛠️ 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.
Contributor
Author
|
/merge |
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
3 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.
Boosts performance when many problems are scheduled in a loop with large matrices