Add matrix interface to problem modeling #775
Conversation
📝 WalkthroughWalkthroughQuadratic objective handling now supports sparse matrix-backed terms via a scipy-based Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Iroy30
added
non-breaking
|
/ok to test 2e8717a |
|
/ok to test 95560f5 |
![coderabbitai[bot]](https://avatars-githubusercontent-com.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
python/cuopt/cuopt/linear_programming/problem.py (3)
1852-1867:Objnow returnsMQuadraticExpression— potential compatibility break.
MQuadraticExpressionlacksQuadraticExpressionaccessors (e.g.,getVariables,getCoefficients,getLinearExpression). Existing clients callingproblem.ObjorgetObjective()for quadratic objectives may fail. Consider returningQuadraticExpressionwhen appropriate, or add a compatibility shim/deprecation path. As per coding guidelines, ...
329-337: UseNoneinstead of mutable default arguments inQuadraticExpression.__init__.Mutable defaults (
vars=[],coefficients=[]) are a Python anti-pattern. Although the current codebase explicitly passes arguments, these defaults could cause unintended state sharing if future code relies on them. Thevarsandcoefficientslists are mutated via.extend()methods, making this a genuine concern.Suggested fix
def __init__( - self, qvars1, qvars2, qcoefficients, vars=[], coefficients=[], constant=0.0 + self, qvars1, qvars2, qcoefficients, vars=None, coefficients=None, constant=0.0 ): + if vars is None: + vars = [] + if coefficients is None: + coefficients = [] self.qvars1 = qvars1 self.qvars2 = qvars2 self.qcoefficients = qcoefficients self.vars = vars self.coefficients = coefficients self.constant = constant
1648-1700: Clear cached quadratic matrices when objective changes to prevent stale data.When an objective is changed from quadratic to linear (or another type), the cached
objective_qcsr_matrixremains and is returned bygetQuadraticObjective(), causing incorrect solver behavior. The matrices must be cleared at the start ofsetObjective()before any case matching. Additionally, fix E128 indentation on thecoo_matrixcall and add explicit shape toMQuadraticExpressionfor consistency withQuadraticExpression.🛠️ Suggested fix
def setObjective(self, expr, sense=MINIMIZE): ... if self.solved: self.reset_solved_values() # Reset all solved values self.ObjSense = sense + # Clear cached quadratic data whenever the objective is reset + self.objective_qcoo_matrix = None + self.objective_qcsr_matrix = None match expr: ... case QuadraticExpression(): ... self.ObjConstant = expr.constant qrows = [var.getIndex() for var in expr.qvars1] qcols = [var.getIndex() for var in expr.qvars2] self.objective_qcoo_matrix = coo_matrix( (np.array(expr.qcoefficients), - (np.array(qrows), np.array(qcols))), - shape=(self.NumVariables, self.NumVariables) + (np.array(qrows), np.array(qcols))), + shape=(self.NumVariables, self.NumVariables), ) case MQuadraticExpression(): ... self.ObjConstant = expr.constant - self.objective_qcoo_matrix = coo_matrix( - expr.qmatrix) + self.objective_qcoo_matrix = coo_matrix( + expr.qmatrix, + shape=(self.NumVariables, self.NumVariables), + )
🤖 Fix all issues with AI agents
In `@conda/recipes/cuopt/recipe.yaml`:
- Line 85: Update the conda recipe dependency for scipy to include a minimum
version constraint (e.g., change the plain "scipy" entry in recipe.yaml to
"scipy >=1.10") to ensure compatibility with code using scipy.sparse.coo_matrix
and scipy.spatial.distance; make the version range consistent with other
scientific deps (for example "scipy >=1.10,<2.0" if you want an upper bound) and
update any CI/test environment metadata to match.
In `@python/cuopt/cuopt/linear_programming/problem.py`:
- Around line 861-872: The inline TODO and commented-out method lines violate
flake8 E265 (missing space after '#'); update the comment lines in this block so
every comment token has a single space after the '#' (e.g., change "## TODO: Add
matrix multiplication" to "# TODO: Add matrix multiplication" and "#def
__matmul__..." to "# def __matmul__(self, qcols):", likewise "#def
__rmatmul__..." to "# def __rmatmul__(self, qcols):") while keeping the same
text and references to the methods __matmul__, __rmatmul__ and attributes
qcols/qrows.
- Around line 746-859: The match statements in __iadd__, __add__, __isub__, and
__sub__ on MQuadraticExpression lack default branches, so unsupported operand
types cause implicit None returns; add a final "case _:" branch in each method
that handles unsupported types by raising a TypeError (or returning
NotImplemented in the non-inplace methods if you prefer Pythonic binary-operator
behavior) with a clear message including the method name and the operand's
actual type (e.g., refer to __iadd__/__isub__/__add__/__sub__,
MQuadraticExpression, Variable, LinearExpression in the message) so the operator
protocol is not violated and errors are explicit.
- Around line 716-741: The constructor of MQuadraticExpression uses mutable
defaults and the qmatrix may be a list-of-lists, causing shared-state bugs and
TypeError in getValue; update __init__ signature to use None for qvars, vars,
and coefficients and inside __init__ set them to [] if None, and also normalize
qmatrix with np.asarray(qmatrix, dtype=float) (store back to self.qmatrix) so
getValue can safely use self.qmatrix[i, j]; ensure constant and other attributes
remain set as before and no other code paths rely on the old default objects.
🧹 Nitpick comments (1)
python/cuopt/cuopt/tests/linear_programming/test_python_API.py (1)
819-848: Consider adding an edge‑case quadratic‑matrix scenario.This test covers a feasible case; please add at least one degenerate/singleton or infeasible/unbounded case (or a variable‑ordering check) to exercise quadratic‑matrix handling under edge conditions. As per coding guidelines, ...
|
/ok to test c2b2032 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
conda/environments/all_cuda-129_arch-x86_64.yaml (1)
1-2: Update the sourcedependencies.yamlfile instead of editing the auto-generated file directly.This file is auto-generated from
../../dependencies.yaml. The header comment instructs that changes must be made to the source file and thenrapids-dependency-file-generatormust be run to regenerate this file. The scipy dependency was added only to this auto-generated file, not to the source. On the next regeneration, this change will be lost.Please add scipy to
dependencies.yamland regenerate this file to ensure the dependency persists.
🤖 Fix all issues with AI agents
In `@conda/environments/all_cuda-131_arch-aarch64.yaml`:
- Line 64: The generated environment includes "scipy" but the source
dependencies.yaml is missing it, so add a scipy entry to dependencies.yaml
(e.g., scipy>=1.15,<2.0 or another range compatible with your NumPy constraint
>=1.23.5,<3.0) and then regenerate the conda environment file so scipy is
preserved; ensure the dependency name matches "scipy" exactly and the version
pin is consistent with Python 3.10–3.13 and aarch64 conda-forge packages.
|
/ok to test 254a5a9 |
chris-maes
left a comment
chris-maes
left a comment
There was a problem hiding this comment.
Could we discuss the need for MQuadraticExpression before merging?
|
/ok to test df85b24 |
|
/ok to test f16a2a9 |
|
/ok to test d42db52 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuopt/cuopt/linear_programming/problem.py (1)
694-777: Replaceif self.qmatrix:withif self.qmatrix is not None:in scalar operations to prevent ValueError on SciPy sparse matrices.SciPy sparse matrices raise
ValueErrorwhen evaluated as boolean (e.g.,if matrix:). The four scalar operations (__imul__,__mul__,__itruediv__,__truediv__) need explicit None checks instead of truthiness checks.🛠️ Proposed fix
- if self.qmatrix: + if self.qmatrix is not None: self.qmatrix *= float(other) ... - if self.qmatrix: + if self.qmatrix is not None: qmatrix = self.qmatrix * float(other) ... - if self.qmatrix: + if self.qmatrix is not None: self.qmatrix = self.qmatrix / float(other) ... - if self.qmatrix: + if self.qmatrix is not None: qmatrix = self.qmatrix / float(other)
♻️ Duplicate comments (2)
python/cuopt/cuopt/linear_programming/problem.py (2)
337-357: Fix mutable defaults and enforceqvarswhenqmatrixis set.The default list arguments (
qvars1,qvars2,qcoefficients,vars,coefficients) can leak state across instances. Also,__len__,getValue, and getters assumeqvarsis present wheneverqmatrixis set—currently aNonedefault can trigger runtime errors.🛠️ Proposed fix
- def __init__( - self, - qmatrix=None, - qvars=None, - qvars1=[], - qvars2=[], - qcoefficients=[], - vars=[], - coefficients=[], - constant=0.0, - ): - self.qmatrix = None - self.qvars = qvars - if qmatrix is not None: - self.qmatrix = coo_matrix(qmatrix) - self.qvars1 = qvars1 - self.qvars2 = qvars2 - self.qcoefficients = qcoefficients - self.vars = vars - self.coefficients = coefficients + def __init__( + self, + qmatrix=None, + qvars=None, + qvars1=None, + qvars2=None, + qcoefficients=None, + vars=None, + coefficients=None, + constant=0.0, + ): + if qmatrix is not None and qvars is None: + raise ValueError("qvars must be provided when qmatrix is set") + self.qmatrix = coo_matrix(qmatrix) if qmatrix is not None else None + self.qvars = list(qvars) if qvars is not None else [] + self.qvars1 = list(qvars1) if qvars1 is not None else [] + self.qvars2 = list(qvars2) if qvars2 is not None else [] + self.qcoefficients = ( + list(qcoefficients) if qcoefficients is not None else [] + ) + self.vars = list(vars) if vars is not None else [] + self.coefficients = ( + list(coefficients) if coefficients is not None else [] + ) self.constant = constantPython mutable default arguments shared state behaviorAlso applies to: 435-436
1603-1624: Validate Q-matrix shape against NumVariables before storing.
expr.qmatrixis added without ensuring it’s square and matchesNumVariables. This can silently create invalid quadratic data and surface as solver failures later. Based on learnings, the solver expects valid dimensions/indices without runtime checks.🛠️ Proposed fix
- if expr.qmatrix is not None: - self.objective_qmatrix += expr.qmatrix + if expr.qmatrix is not None: + qcoo = coo_matrix(expr.qmatrix) + if qcoo.shape[0] != qcoo.shape[1]: + raise ValueError("qmatrix must be square") + if qcoo.shape[0] != self.NumVariables: + raise ValueError( + "qmatrix size must match number of variables" + ) + if expr.qvars and len(expr.qvars) != self.NumVariables: + raise ValueError( + "qvars length must match number of variables" + ) + self.objective_qmatrix += qcooBased on learnings, the solver expects valid dimensions/indices without runtime checks.
rg20
changed the title
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Awesome work @Iroy30, Have few questions and suggestions. Rest looks good.
| >>> quad_expr = QuadraticExpression( | ||
| ... [x, x], [x, y], [1.0, 2.0], | ||
| ... [x], [3.0], 4.0 | ||
| ... qmatrix=[[1.0, 2.0], [0.0, 0.0]], |
There was a problem hiding this comment.
Lets add example for both qmatrix usage and qvars1 usage, Also provide a bit more detail in description about how these are used.
|
|
||
| Parameters | ||
| ---------- | ||
| qmatrix : List[List[float]] or 2D numpy array. |
There was a problem hiding this comment.
We are missing parameter qvars
| for i, var1 in enumerate(self.qvars1): | ||
| var2 = self.qvars2[i] | ||
| value += var1.Value * var2.Value * self.qcoefficients[i] | ||
| if self.qmatrix is not None: |
There was a problem hiding this comment.
Should this also checj qvars is not None ?
There was a problem hiding this comment.
qmatrix and qvars go hand in hand, If self.qmatrix is not None implies self.qvars is not None
There was a problem hiding this comment.
Lets check in init that both are getting initialized, else error
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@python/cuopt/cuopt/linear_programming/problem.py`:
- Around line 1843-1849: getQCSR() assumes self.objective_qmatrix exists and
will raise AttributeError for purely linear problems; add an explicit guard at
the start of getQCSR that checks for the presence/non-None of
self.objective_qmatrix and if missing raises a clear exception (e.g., ValueError
or custom error) with a message like "No quadratic objective set" instead of
letting an AttributeError propagate; keep the existing construction of
qcsr_matrix and return via self.dict_to_object unchanged when the guard passes.
- Around line 733-735: The code currently uses truthiness checks like "if
self.qmatrix:" which raises ValueError for SciPy sparse matrices; update all
such checks to explicit None checks (e.g., "if self.qmatrix is not None:") in
the methods that modify or operate on the quadratic matrix (the in-place scalar
multiply block that does "self.qmatrix *= float(other)" and the similar checks
at the other occurrences), so the sparse coo_matrix isn't evaluated for
truthiness; apply the same change to every occurrence mentioned (the blocks
around self.qmatrix at lines referenced) to prevent crashes.
- Around line 463-465: The __len__ method currently does len(self.qvars) which
fails when self.qvars is None and miscounts quadratic terms; change it to sum
the number of variables (len(self.vars)), the number of triplet quadratic
entries (len(self.qvars1)), and the number of nonzero matrix quadratic entries
using self.qmatrix.nnz (or 0 if self.qmatrix is None/not present) — update the
__len__ implementation in the class to return len(self.vars) + len(self.qvars1)
+ (self.qmatrix.nnz if getattr(self, "qmatrix", None) is not None else 0).
🧹 Nitpick comments (1)
python/cuopt/cuopt/linear_programming/problem.py (1)
1311-1313: Add deprecation shim if old quadratic-objective fields were public.Line 1311-1313 introduces
objective_qmatrix. Ifobjective_qcoo_matrix/objective_qcsr_matrixwere part of the public Python API, consider keeping deprecated aliases (with warnings) to avoid breaking users while they migrate. As per coding guidelines, ensure backward compatibility with deprecation warnings.
|
/ok to test da78edf |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@python/cuopt/cuopt/linear_programming/problem.py`:
- Around line 1701-1707: The deprecation warning strings in get_incumbent_values
(and similarly in get_pdlp_warm_start_data and getQcsr) concatenate adjacent
literals without a separating space, producing "...removed.Please use...";
update the DeprecationWarning message in get_incumbent_values to insert a space
(or newline) between the two sentences so the text reads "...removed. Please use
getIncumbentValues instead.", and apply the same fix to the warning messages in
get_pdlp_warm_start_data and getQcsr to ensure proper spacing and readability.
| def get_incumbent_values(self, solution, vars): | ||
| warnings.warn( | ||
| "This function is deprecated and will be removed." | ||
| "Please use getIncumbentValues instead.", | ||
| DeprecationWarning, | ||
| ) | ||
| return self.getIncumbentValues(solution, vars) |
There was a problem hiding this comment.
Add space between sentences in deprecation warning messages.
Adjacent string literals are concatenated without whitespace, producing messages like "...removed.Please use...". Add a space or newline for readability.
🛠️ Suggested fix
def get_incumbent_values(self, solution, vars):
warnings.warn(
- "This function is deprecated and will be removed."
- "Please use getIncumbentValues instead.",
+ "This function is deprecated and will be removed. "
+ "Please use getIncumbentValues instead.",
DeprecationWarning,
)
return self.getIncumbentValues(solution, vars)The same fix applies to get_pdlp_warm_start_data (line 1729-1732) and getQcsr (line 1874-1877).
🤖 Prompt for AI Agents
In `@python/cuopt/cuopt/linear_programming/problem.py` around lines 1701 - 1707,
The deprecation warning strings in get_incumbent_values (and similarly in
get_pdlp_warm_start_data and getQcsr) concatenate adjacent literals without a
separating space, producing "...removed.Please use..."; update the
DeprecationWarning message in get_incumbent_values to insert a space (or
newline) between the two sentences so the text reads "...removed. Please use
getIncumbentValues instead.", and apply the same fix to the warning messages in
get_pdlp_warm_start_data and getQcsr to ensure proper spacing and readability.
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Have few minor suggestions and questions, once you address it, I think we are good.
| @@ -0,0 +1,80 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & | |||
There was a problem hiding this comment.
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & | |
| # SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & |
There was a problem hiding this comment.
Updated all examples copyright
| @@ -0,0 +1,80 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
| # SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
Updated all examples copyright
|
/ok to test 955f640 |
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Awesome work @Iroy30
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@python/cuopt/cuopt/linear_programming/problem.py`:
- Around line 1643-1654: Validate that expr.qvars ordering matches the problem
variable order before adding expr.qmatrix to objective_qmatrix: in the block
that constructs self.objective_qmatrix (around the use of qrows/qcols and
expr.qmatrix), compare expr.qvars (or their indices via var.getIndex())
element-wise against self.vars (or their indices) and raise a clear ValueError
if they differ (include which variable/order mismatch). This ensures the
quadratic matrix being added aligns with self.objective_qmatrix; keep the
existing length check but add this ordering check (referencing expr.qvars,
self.vars, self.objective_qmatrix, and QuadraticExpression) and prevent the
silent misalignment by failing fast with an explanatory error.
🧹 Nitpick comments (3)
python/cuopt/cuopt/linear_programming/problem.py (2)
360-387: Mutable default arguments can cause shared state bugs.The
__init__method uses mutable default arguments (qvars=[],qvars1=[],qvars2=[],qcoefficients=[],vars=[],coefficients=[]). If these lists are ever mutated (e.g., viaappendorextend), the same list object will be shared across all instances that use the default. While the current implementation assigns these directly without mutation in__init__, this is a latent bug waiting to happen if code changes.♻️ Safer approach using None defaults
def __init__( self, qmatrix=None, - qvars=[], - qvars1=[], - qvars2=[], - qcoefficients=[], - vars=[], - coefficients=[], + qvars=None, + qvars1=None, + qvars2=None, + qcoefficients=None, + vars=None, + coefficients=None, constant=0.0, ): self.qmatrix = None - self.qvars = qvars + self.qvars = qvars if qvars is not None else [] if qmatrix is not None: self.qmatrix = coo_matrix(qmatrix) mshape = self.qmatrix.shape if mshape[0] != mshape[1]: raise ValueError("qmatrix should be a square matrix") if len(qvars) != mshape[0]: raise ValueError( "qvars length mismatch. Should match with qmatrix length. Please check docs for more details." ) - self.qvars1 = qvars1 - self.qvars2 = qvars2 - self.qcoefficients = qcoefficients - self.vars = vars - self.coefficients = coefficients + self.qvars1 = qvars1 if qvars1 is not None else [] + self.qvars2 = qvars2 if qvars2 is not None else [] + self.qcoefficients = qcoefficients if qcoefficients is not None else [] + self.vars = vars if vars is not None else [] + self.coefficients = coefficients if coefficients is not None else [] self.constant = constant
465-466:__len__counts variables instead of quadratic terms for matrix-based expressions.For matrix-based expressions,
len(self.qvars)returns the number of variables (matrix dimension), not the number of quadratic terms. This is inconsistent with the triplet case wherelen(self.qvars1)counts actual quadratic terms. Consider usingself.qmatrix.nnzto count non-zero matrix entries.♻️ Proposed fix
def __len__(self): - return len(self.vars) + len(self.qvars1) + len(self.qvars) + q_terms = len(self.qvars1) + if self.qmatrix is not None: + q_terms += self.qmatrix.nnz + return len(self.vars) + q_termsdocs/cuopt/source/cuopt-python/lp-qp-milp/examples/qp_matrix_example.py (1)
56-61: Add status check before accessing solution values for consistency.Unlike
solution_example.pywhich checksproblem.Status.name == "Optimal"before accessing results, this example assumes the solve succeeded. For robustness and consistency across examples, consider adding a status check.♻️ Suggested improvement
# Solve the problem prob.solve() # Print results - print(f"Optimal solution found in {prob.SolveTime:.2f} seconds") - print(f"p1 = {p1.Value}") - print(f"p2 = {p2.Value}") - print(f"p3 = {p3.Value}") - print(f"Minimized cost = {prob.ObjValue}") + if prob.Status.name == "Optimal": + print(f"Optimal solution found in {prob.SolveTime:.2f} seconds") + print(f"p1 = {p1.Value}") + print(f"p2 = {p2.Value}") + print(f"p3 = {p3.Value}") + print(f"Minimized cost = {prob.ObjValue}") + else: + print(f"Problem status: {prob.Status.name}")
| qrows = [var.getIndex() for var in expr.qvars1] | ||
| qcols = [var.getIndex() for var in expr.qvars2] | ||
| self.objective_qmatrix = coo_matrix( | ||
| ( | ||
| np.array(expr.qcoefficients), | ||
| (np.array(qrows), np.array(qcols)), | ||
| ), | ||
| shape=(self.NumVariables, self.NumVariables), | ||
| ) | ||
| if expr.qmatrix is not None: | ||
| self.objective_qmatrix += expr.qmatrix | ||
| self.objective_qmatrix = self.objective_qmatrix.tocsr() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full setObjective method context
cat -n python/cuopt/cuopt/linear_programming/problem.py | sed -n '1620,1680p'Repository: NVIDIA/cuopt
Length of output: 3280
🏁 Script executed:
# Search for qmatrix usage and validation in the file
rg -n "qmatrix" python/cuopt/cuopt/linear_programming/problem.py -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 10295
🏁 Script executed:
# Search for qvars validation or ordering checks
rg -n "qvars|expr\.qmatrix" python/cuopt/cuopt/linear_programming/problem.py -B 3 -A 3Repository: NVIDIA/cuopt
Length of output: 15097
🏁 Script executed:
# Look for test files that test qmatrix functionality
fd -t f "test.*\.py" --search-path python/ | xargs rg -l "qmatrix\|quadratic.*objective" 2>/dev/null | head -10Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check if there's any re-indexing logic for qmatrix when variables are reordered
rg -n "reindex|remap|transpose" python/cuopt/cuopt/linear_programming/problem.pyRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Look for any variable reordering or mapping logic in the file
rg -n "permute|mapping\|map_var\|var.*order" python/cuopt/cuopt/linear_programming/problem.pyRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Examine how getObjective reconstructs the QuadraticExpression to understand index assumptions
cat -n python/cuopt/cuopt/linear_programming/problem.py | sed -n '1827,1840p'Repository: NVIDIA/cuopt
Length of output: 632
🏁 Script executed:
# Search for test files covering quadratic objective matrix with mismatched variable ordering
fd -t f "test.*\.py\|.*_test\.py" --search-path python/ | xargs grep -l "qmatrix\|quadratic" 2>/dev/nullRepository: NVIDIA/cuopt
Length of output: 40
🏁 Script executed:
# Look for any bug reports or issues related to qmatrix ordering
rg -n "qmatrix.*order\|order.*qvars\|variable.*order.*qmatrix" python/cuopt/ --type pyRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check if the issue is acknowledged or handled elsewhere
rg -n "qvars.*order\|order.*variables" python/cuopt/cuopt/linear_programming/problem.py -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Let's verify by checking if there's any scenario where variable ordering could differ
# Look at how variables are added and if their indices can be non-monotonic
rg -n "addVariable|getIndex" python/cuopt/cuopt/linear_programming/problem.py | head -20Repository: NVIDIA/cuopt
Length of output: 1265
🏁 Script executed:
# Check the Variable class to understand getIndex() behavior
fd -t f "variable*.py" --search-path python/cuopt/Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Find Variable class definition
rg -n "class Variable" python/cuopt/ -A 5Repository: NVIDIA/cuopt
Length of output: 623
🏁 Script executed:
# Check how getIndex is implemented
rg -n "def getIndex" python/cuopt/cuopt/linear_programming/problem.py -A 3Repository: NVIDIA/cuopt
Length of output: 182
🏁 Script executed:
# Examine the addVariable method to see how indices are assigned
cat -n python/cuopt/cuopt/linear_programming/problem.py | sed -n '1479,1530p'Repository: NVIDIA/cuopt
Length of output: 2023
🏁 Script executed:
# Final check: Look at how qmatrix is used in getValue() to understand index assumption
cat -n python/cuopt/cuopt/linear_programming/problem.py | sed -n '449,463p'Repository: NVIDIA/cuopt
Length of output: 819
🏁 Script executed:
# Check if there are any tests that mix qmatrix with variable subsets
find python -name "*.py" -type f -exec grep -l "qmatrix" {} \; 2>/dev/null | xargs grep -l "test\|Test" 2>/dev/null | head -3Repository: NVIDIA/cuopt
Length of output: 118
🏁 Script executed:
# Check the test file for qmatrix tests
grep -n "qmatrix" python/cuopt/cuopt/tests/linear_programming/test_python_API.py -B 3 -A 5 | head -40Repository: NVIDIA/cuopt
Length of output: 926
🏁 Script executed:
# Check if there are any other tests with qmatrix and variable reordering
grep -n "QuadraticExpression.*qmatrix\|qmatrix.*qvars" python/cuopt/cuopt/tests/linear_programming/test_python_API.py -B 2 -A 3Repository: NVIDIA/cuopt
Length of output: 375
Add validation of qvars ordering when setting quadratic objective matrix.
At line 1653, expr.qmatrix is added directly to objective_qmatrix without verifying that expr.qvars ordering matches self.vars ordering. While the documentation (lines 318-320 in QuadraticExpression) requires qvars to match problem variable order, the constructor only validates length, not ordering. If a user provides correctly-sized qvars in a different order, the matrix indices become misaligned with problem variables, causing silent incorrect quadratic terms. Add validation to ensure expr.qvars matches self.vars element-wise before the matrix addition, or document the strict requirement more prominently with examples showing correct usage.
🤖 Prompt for AI Agents
In `@python/cuopt/cuopt/linear_programming/problem.py` around lines 1643 - 1654,
Validate that expr.qvars ordering matches the problem variable order before
adding expr.qmatrix to objective_qmatrix: in the block that constructs
self.objective_qmatrix (around the use of qrows/qcols and expr.qmatrix), compare
expr.qvars (or their indices via var.getIndex()) element-wise against self.vars
(or their indices) and raise a clear ValueError if they differ (include which
variable/order mismatch). This ensures the quadratic matrix being added aligns
with self.objective_qmatrix; keep the existing length check but add this
ordering check (referencing expr.qvars, self.vars, self.objective_qmatrix, and
QuadraticExpression) and prevent the silent misalignment by failing fast with an
explanatory error.
|
/merge |
rapids-bot
bot
merged commit 7995486
into
NVIDIA:release/26.02
6 participants
Description
Issue
Checklist
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.