Add addMatrixConsDisjunction for matrix constraints#1210
Add addMatrixConsDisjunction for matrix constraints#1210Joao-Dionisio wants to merge 3 commits intomasterfrom
Conversation
…ction # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Pull request overview
Adds a dedicated Model.addMatrixConsDisjunction() API to support elementwise disjunctions when working with matrix constraint expressions (MatrixExprCons), addressing a gap in matrix-constraint composition.
Changes:
- Implement
Model.addMatrixConsDisjunction()inscip.pxi, creating one disjunction constraint per matrix index (with scalar fallback). - Add regression tests covering shape mismatch/type validation and optimization behavior for matrix + scalar fallback.
- Expose the method in type stubs and document it in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/pyscipopt/scip.pxi |
Implements addMatrixConsDisjunction() including broadcasting and scalar fallback logic. |
tests/test_matrix_variable.py |
Adds test coverage for matrix disjunction behavior and error paths. |
src/pyscipopt/scip.pyi |
Adds stub entry for the new Model method. |
CHANGELOG.md |
Documents the new API addition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dynamic: Union[bool, np.ndarray] = False): | ||
| """ | ||
| Add an elementwise disjunction of matrix constraints. | ||
|
|
||
| Given an iterable of ``MatrixExprCons`` with a common shape, creates one | ||
| disjunction per index: for each position ``idx``, the resulting | ||
| disjunction enforces that at least one of ``conss[k][idx]`` holds. | ||
| ``ExprCons`` entries are broadcast to every position. If every entry in | ||
| ``conss`` is a plain ``ExprCons``, the call is forwarded to | ||
| :meth:`addConsDisjunction` and returns a single ``Constraint``. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| conss : iterable of MatrixExprCons or ExprCons | ||
| Constraints to combine elementwise into disjunctions. All | ||
| ``MatrixExprCons`` entries must share the same shape. |
There was a problem hiding this comment.
The new API is named/worded as if it works with already-added MatrixConstraint objects (like those returned by addMatrixCons), but the implementation only accepts MatrixExprCons/ExprCons. This means the #1084 reproduction (building c1 = addMatrixCons(...) then passing c1 into a disjunction) still won’t work unless users change how they call the API. Consider either (a) extending this method to also accept MatrixConstraint/Constraint inputs (elementwise) or (b) explicitly documenting here that callers must pass constraint expressions (e.g., A @ x == b) rather than constraints returned by addMatrixCons.
| - `Expr` and `GenExpr` support NumPy unary functions (`np.sin`, `np.cos`, `np.sqrt`, `np.exp`, `np.log`, `np.absolute`) | ||
| - Added `getBase()` and `setBase()` methods to `LP` class for getting/setting basis status | ||
| - Added `getMemUsed()`, `getMemTotal()`, and `getMemExternEstim()` methods | ||
| - Added `addMatrixConsDisjunction()` for elementwise disjunctions over matrix constraints (#1084) |
There was a problem hiding this comment.
Changelog entry says “disjunctions over matrix constraints”, which can be read as supporting MatrixConstraint objects (returned by addMatrixCons). Since the new function currently takes MatrixExprCons/ExprCons, consider adjusting the wording to “matrix constraint expressions”/“MatrixExprCons” (or similar) so users don’t expect addConsDisjunction([c1, c2]) with c1/c2 being MatrixConstraint to work.
| - Added `addMatrixConsDisjunction()` for elementwise disjunctions over matrix constraints (#1084) | |
| - Added `addMatrixConsDisjunction()` for elementwise disjunctions over matrix constraint expressions (`MatrixExprCons`/`ExprCons`) (#1084) |
| if all(isinstance(cons, ExprCons) for cons in conss): | ||
| return self.addConsDisjunction( | ||
| conss, name=name, initial=initial, relaxcons=relaxcons, | ||
| enforce=enforce, check=check, local=local, | ||
| modifiable=modifiable, dynamic=dynamic, | ||
| ) |
There was a problem hiding this comment.
In the all(ExprCons) fallback, this forwards name/initial/enforce/... directly to addConsDisjunction. If a caller passes NumPy arrays for these (allowed by this method’s signature), the scalar call will receive an np.ndarray where it expects a str/bool, leading to a runtime error. Consider validating that all matrix-style parameters are scalars in the scalar-fallback path (or reducing them to a scalar explicitly) and raising a clear TypeError/ValueError otherwise.
…not Constraint objects
Zeroto521
left a comment
Zeroto521
left a comment
There was a problem hiding this comment.
I haven't used the addConsDisjunction API before, so I cannot provide much useful advice here.
| ``conss`` is a ``MatrixExprCons``; otherwise a single | ||
| ``Constraint`` from the scalar fallback. | ||
| """ | ||
| assert isinstance(conss, Iterable), "Given constraint list is not iterable" |
There was a problem hiding this comment.
It would be better to use raise instead of assert in the main code.
| assert isinstance(conss, Iterable), "Given constraint list is not iterable" | |
| if not isinstance(conss, Iterable): | |
| raise TypeError(f"given constraint list is not iterable, but got {type(conss).__name__}") |
| matrix_cons = np.empty(shape, dtype=object) | ||
| for idx in np.ndindex(shape): | ||
| elem_conss = [ | ||
| cons[idx] if isinstance(cons, MatrixExprCons) else cons | ||
| for cons in conss | ||
| ] | ||
| matrix_cons[idx] = self.addConsDisjunction( | ||
| elem_conss, | ||
| name=matrix_names[idx], | ||
| initial=matrix_initial[idx], | ||
| relaxcons=relaxcons, | ||
| enforce=matrix_enforce[idx], | ||
| check=matrix_check[idx], | ||
| local=matrix_local[idx], | ||
| modifiable=matrix_modifiable[idx], | ||
| dynamic=matrix_dynamic[idx], | ||
| ) |
There was a problem hiding this comment.
It's too slow to use Python loops to build matrices when using addMatrixCons and addMatrixConsIndicator for large variables. I'm planning to port this to Cython later for a speed boost, but it works fine as-is for now.
Fixes #1084.
@Zeroto521 you're our matrix guy, would you mind taking a look? :)