Add matrix interface to problem modeling #775
Conversation
📝 WalkthroughWalkthroughReworked quadratic objective handling to use a COO/CSR-backed 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/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 |
@Iroy30, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
There was a problem hiding this comment.
Actionable comments posted: 2
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)
1653-1656: Invalidate cached quadratic matrices whenever the objective changes.If
getQcsr()has been called, changing the objective before solve leavesobjective_qcsr_matrixstale. Also, switching from quadratic → linear keeps the old Q unless explicitly cleared.🛠️ Proposed fix
def setObjective(self, expr, sense=MINIMIZE): ... if self.solved: self.reset_solved_values() # Reset all solved values + # Objective updates should invalidate cached quadratic matrices + self.objective_qcoo_matrix = None + self.objective_qcsr_matrix = None self.ObjSense = sense
🤖 Fix all issues with AI agents
In `@python/cuopt/cuopt/linear_programming/problem.py`:
- Around line 1695-1706: When handling MQuadraticExpression, validate
expr.qmatrix before assigning to self.objective_qcoo_matrix: compute num_vars =
len(self.vars) (or use self.NumVariables if present), convert expr.qmatrix to a
coo_matrix temporarily, then check that its shape is square (shape[0] ==
shape[1]) and equals num_vars; if not, raise an informative exception (e.g.,
ValueError) rejecting the Q-matrix; optionally also check that all row/col
indices are within [0, num_vars) before setting self.objective_qcoo_matrix =
coo_matrix(expr.qmatrix).
In `@python/cuopt/pyproject.toml`:
- Line 35: The pyproject.toml currently pins "scipy>=1.13.0" which has no wheels
for Python 3.13; update the dependency to "scipy>=1.14.1" to restore Python 3.13
compatibility, or alternatively remove Python 3.13 from the package's
requires-python and classifiers so declared Python versions match supported
SciPy wheels — edit the scipy requirement line ("scipy>=1.13.0") and the
requires-python/classifiers entries accordingly to keep them consistent.
♻️ Duplicate comments (3)
python/cuopt/cuopt/linear_programming/problem.py (3)
722-746:MQuadraticExpressionstill shares state and fails on list-of-lists qmatrix.Defaults are mutable, and
self.qmatrix[i, j]fails for list-of-lists inputs. Please normalize tonp.asarrayand useNonedefaults.🛠️ Proposed fix
- def __init__( - self, qmatrix, qvars=[], vars=[], coefficients=[], constant=0.0 - ): - self.qmatrix = qmatrix - self.qvars = qvars - self.vars = vars - self.coefficients = coefficients + def __init__( + self, qmatrix, qvars=None, vars=None, coefficients=None, constant=0.0 + ): + self.qmatrix = np.asarray(qmatrix, dtype=float) + self.qvars = list(qvars) if qvars is not None else [] + self.vars = list(vars) if vars is not None else [] + self.coefficients = ( + list(coefficients) if coefficients is not None else [] + ) + if self.qmatrix.ndim != 2 or self.qmatrix.shape[0] != self.qmatrix.shape[1]: + raise ValueError("qmatrix must be square") + if self.qvars and self.qmatrix.shape[0] != len(self.qvars): + raise ValueError("qmatrix size must match qvars length") self.constant = constant
751-865: Handle unsupported operand types in MQuadraticExpression operators.
__iadd__,__add__,__isub__, and__sub__fall through without a default case, returningNoneand breaking operator semantics. Add explicit error handling (orNotImplemented).🧩 Suggested pattern
def __iadd__(self, other): # Compute quad_expr += lin_expr match other: case int() | float(): ... return self case Variable(): ... return self case LinearExpression(): ... return self + case _: + raise TypeError( + "unsupported operand type(s) for +=: " + "'MQuadraticExpression' and '%s'" + % type(other).__name__ + )Apply the same pattern to
__add__,__isub__, and__sub__.
329-336: Fix mutable default arguments inQuadraticExpressionandMQuadraticExpression.Both classes use shared mutable default lists (
vars=[],coefficients=[]inQuadraticExpressionat lines 329-336;qvars=[],vars=[],coefficients=[]inMQuadraticExpressionat lines 726-730) with direct assignment, which can leak state between instances. UseNonedefaults and instantiate new lists instead.Proposed fixes
For
QuadraticExpression:- def __init__( - self, - qvars1, - qvars2, - qcoefficients, - vars=[], - coefficients=[], - constant=0.0, - ): - self.vars = vars - self.coefficients = coefficients + def __init__( + self, + qvars1, + qvars2, + qcoefficients, + vars=None, + coefficients=None, + constant=0.0, + ): + self.vars = list(vars) if vars is not None else [] + self.coefficients = ( + list(coefficients) if coefficients is not None else [] + )For
MQuadraticExpression:- def __init__( - self, qmatrix, qvars=[], vars=[], coefficients=[], constant=0.0 - ): + def __init__( + self, qmatrix, qvars=None, vars=None, coefficients=None, constant=0.0 + ): self.qmatrix = qmatrix - self.qvars = qvars - self.vars = vars - self.coefficients = coefficients + self.qvars = list(qvars) if qvars is not None else [] + self.vars = list(vars) if vars is not None else [] + self.coefficients = ( + list(coefficients) if coefficients is not None else [] + )
🧹 Nitpick comments (2)
python/cuopt/cuopt/tests/linear_programming/test_python_API.py (1)
819-848: Add a degenerate/singleton Q-matrix test to complement the nominal case.This new test validates a typical quadratic objective. Consider adding a singleton or zero-matrix case to ensure dimension handling and conversions behave for edge sizes. As per coding guidelines, test degenerate/singleton problems as well.
python/cuopt/cuopt/linear_programming/problem.py (1)
1864-1877: Avoid densifying Q inObj/ObjValuefor large models.
objective_qcoo_matrix.todense()can explode memory for large problems. Consider returning a sparse-backed expression (or a lightweight view) to avoid full densification duringObj/ObjValueaccess.
| case MQuadraticExpression(): | ||
| for var in self.vars: | ||
| var.setObjectiveCoefficient(0.0) | ||
| for var, coeff in zip(expr.vars, expr.coefficients): | ||
| c_val = self.vars[var.getIndex()].getObjectiveCoefficient() | ||
| sum_coeff = coeff + c_val | ||
| self.vars[var.getIndex()].setObjectiveCoefficient( | ||
| sum_coeff | ||
| ) | ||
| self.ObjConstant = expr.constant | ||
| self.objective_qcoo_matrix = coo_matrix(expr.qmatrix) | ||
| case _: |
There was a problem hiding this comment.
Validate Q-matrix shape against the problem before storing it.
coo_matrix(expr.qmatrix) is accepted without checking that it’s square and matches NumVariables, which can silently generate invalid Q. Add shape checks and reject mismatches early. Based on learnings, the solver expects valid dimensions/indices without runtime validation.
🛠️ Proposed fix
case MQuadraticExpression():
...
self.ObjConstant = expr.constant
- self.objective_qcoo_matrix = coo_matrix(expr.qmatrix)
+ 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_qcoo_matrix = qcoo🤖 Prompt for AI Agents
In `@python/cuopt/cuopt/linear_programming/problem.py` around lines 1695 - 1706,
When handling MQuadraticExpression, validate expr.qmatrix before assigning to
self.objective_qcoo_matrix: compute num_vars = len(self.vars) (or use
self.NumVariables if present), convert expr.qmatrix to a coo_matrix temporarily,
then check that its shape is square (shape[0] == shape[1]) and equals num_vars;
if not, raise an informative exception (e.g., ValueError) rejecting the
Q-matrix; optionally also check that all row/col indices are within [0,
num_vars) before setting self.objective_qcoo_matrix = coo_matrix(expr.qmatrix).
|
/ok to test 9ddfca4 |
|
/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
| coefficients=[], | ||
| constant=0.0, | ||
| ): | ||
| self.qmatrix = None |
There was a problem hiding this comment.
If qmatrix is not None can qvars be NOne ?
| return self.qvars2[i] | ||
| qvars2 = self.qvars2 | ||
| if self.qmatrix is not None: | ||
| qvars2 = self.qvars2 + [self.qvars[c] for c in self.qmatrix.col] |
There was a problem hiding this comment.
Should this also checj qvars is not None ?
| return self.qvars1[i] | ||
| qvars1 = self.qvars1 | ||
| if self.qmatrix is not None: | ||
| qvars1 = self.qvars1 + [self.qvars[r] for r in self.qmatrix.row] |
There was a problem hiding this comment.
Should this also checj qvars is not None ?
| qvars1 = self.qvars1 | ||
| qvars2 = self.qvars2 | ||
| if self.qmatrix is not None: | ||
| qvars1 = self.qvars1 + [self.qvars[i] for i in self.qmatrix.row] |
There was a problem hiding this comment.
Should this also checj qvars is not None ?
| 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
| qmatrix = self.qmatrix | ||
| qvars = self.qvars | ||
| if self.qmatrix is not None and other.qmatrix is not None: | ||
| qmatrix = self.qmatrix + other.qmatrix |
There was a problem hiding this comment.
Would + append in case a 2D arrays or add to each other ?
There was a problem hiding this comment.
self.qmatrix is in scipy coo_matrix format. Addition of two scipy coo_matrix adds both matrices and returns scipy matrix type
| dm.set_objective_coefficients(self.objective) | ||
| dm.set_objective_offset(self.ObjConstant) | ||
| if self.getQcsr(): | ||
| if self.objective_qmatrix is not None: |
There was a problem hiding this comment.
If this is not, we don't set objective matrix ?
There was a problem hiding this comment.
If self.objective_qmatrix is None, no quatratic terms , either matrix or expression, are present in the model. In that case we don't call the quadratic objective setter APIs. (Note that self.objective_qmatrix is formulated when setObjective() is called and all the quadratic Expression as well as Matrix is added up into a final objective_qmatrix). The linear terms are added as usual in the above lines.
6 participants
Description
Issue
Checklist
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.