BUG: MatrixExpr can't be compared with Expr #1069

Zeroto521 · 2025-09-04T10:11:16Z

Updated type hints and isinstance checks in MatrixExpr comparison methods to use Expr instead of Variable. This change improves compatibility with broader expression types in matrix operations.

Updated type hints and isinstance checks in MatrixExprCons.__le__ and __ge__ methods to use Expr instead of Variable. This change improves consistency with the expected types for matrix expression constraints.

Introduces test_ranged_matrix_cons to verify correct behavior when adding a ranged matrix constraint to the model. Ensures that the matrix variable x is set to ones as expected.

Introduced a shared _matrixexpr_richcmp helper to handle rich comparison logic for MatrixExpr and MatrixExprCons, reducing code duplication and improving maintainability. Updated __le__, __ge__, and __eq__ methods to use this helper, and removed redundant code.

The __eq__ method of MatrixExprCons now raises NotImplementedError with a descriptive message instead of TypeError, clarifying that '==' comparison is not supported.

Added tests for '<=', '>=', and '==' operators in matrix constraints. Verified correct exception is raised for unsupported '==' operator.

Relocated the _is_number utility from expr.pxi to matrix.pxi for better modularity. Updated _matrixexpr_richcmp to use a local _richcmp helper for comparison operations.

Replaces usage of undefined 'shape' variable with 'self.shape' when creating the result array in _matrixexpr_richcmp, ensuring correct array dimensions.

Zeroto521 · 2025-09-04T10:29:57Z

src/pyscipopt/expr.pxi

-def _is_number(e):
-    try:
-        f = float(e)
-        return True
-    except ValueError: # for malformed strings
-        return False
-    except TypeError: # for other types (Variable, Expr)
-        return False
-


Remove duplicated parts. It also appears in matrix.pxi

I would rather have this code be here than in matrix.pxi

Zeroto521 · 2025-09-04T10:31:13Z

src/pyscipopt/matrix.pxi

+    def _richcmp(self, other, op):
+        if op == 1: # <=
+            return self.__le__(other)
+        elif op == 5: # >=
+            return self.__ge__(other)
+        elif op == 2: # ==
+            return self.__eq__(other)
+        else:
+            raise NotImplementedError("Can only support constraints with '<=', '>=', or '=='.")


Can't use expr.pxi/_expr_richcmp. It will cause circular imports

Removed unnecessary double .all() calls in assertions for matrix variable tests, simplifying the checks for equality with np.ones(3).

Updated assertions in test_matrix_variable.py to use m.getVal(x) and m.getVal(y) instead of direct variable comparison. This ensures the tests check the evaluated values from the model rather than the symbolic variables.

Zeroto521 · 2025-09-04T10:45:38Z

src/pyscipopt/matrix.pxi

-        if not _is_number(other) or not isinstance(other, MatrixExpr):
-                raise TypeError('Ranged MatrixExprCons is not well defined!')


This checking is duplicated to _expr_richcmp

…dundant checks

This reverts commit f69ce7e.

This reverts commit b6dcf42.

Can't add with ExprCons

Zeroto521 · 2025-09-04T14:09:55Z

src/pyscipopt/matrix.pxi

-                expr_cons_matrix[idx] = self[idx] >= other[idx]    
-        else:
-            raise TypeError(f"Unsupported type {type(other)}")
+    def __le__(self, other: Union[float, int, np.ndarray]) -> MatrixExprCons:


Ranged ExprCons can only support numbers.
So MatrixExprCons.__ge__ can only receive a number type other.

PySCIPOpt/src/pyscipopt/expr.pxi

Lines 334 to 356 in c681f94

def __richcmp__(self, other, op):

'''turn it into a constraint'''

if op == 1: # <=

if not self._rhs is None:

raise TypeError('ExprCons already has upper bound')

assert not self._lhs is None

if not _is_number(other):

raise TypeError('Ranged ExprCons is not well defined!')

return ExprCons(self.expr, lhs=self._lhs, rhs=float(other))

elif op == 5: # >=

if not self._lhs is None:

raise TypeError('ExprCons already has lower bound')

assert self._lhs is None

assert not self._rhs is None

if not _is_number(other):

raise TypeError('Ranged ExprCons is not well defined!')

return ExprCons(self.expr, lhs=float(other), rhs=self._rhs)

else:

raise TypeError

A toy demo to show that

from pyscipopt import Model m = Model() x = m.addVar(vtype="B", ub=0) y = m.addVar(vtype="B", ub=0) # (x <= 1) >= y # left is (x <= 1) (ExprCons), right is y (Variable) # Traceback (most recent call last): # line 6, in <module> # (x <= 1) >= y # File "src/pyscipopt/expr.pxi", line 352, in pyscipopt.scip.ExprCons.__richcmp__ # TypeError: Ranged ExprCons is not well defined! y <= (x <= 1) # left is y (Variable), right is (x <= 1) (ExprCons) # Traceback (most recent call last): # line 12, in <module> # y <= (x <= 1) # File "src/pyscipopt/expr.pxi", line 287, in pyscipopt.scip.Expr.__richcmp__ # File "src/pyscipopt/expr.pxi", line 65, in pyscipopt.scip._expr_richcmp # NotImplementedError

Zeroto521 · 2025-09-04T14:12:29Z

src/pyscipopt/expr.pxi

-           if not self._rhs is None:
-               raise TypeError('ExprCons already has upper bound')
-           assert not self._lhs is None
+            if not self._rhs is None:


Use 4 spaces as the indent

jonathanberthias · 2025-09-16T21:19:37Z

This is great @Zeroto521, I just hit the same issue so thanks a lot for working on this!
Just a quick question, would any more changes be needed to make comparison with GenExprs work?

Zeroto521 · 2025-09-17T05:07:05Z

This is great @Zeroto521, I just hit the same issue so thanks a lot for working on this! Just a quick question, would any more changes be needed to make comparison with GenExprs work?

Expr and GenExpr are two different classes. The relationships between Variable, Term, Expr, and GenExpr are kind of complex. I want to merge Expr and GenExpr and simplify the inheritance.
This pr only helps for Expr now.

Joao-Dionisio · 2025-10-17T09:06:13Z

src/pyscipopt/matrix.pxi

+
+def _matrixexpr_richcmp(self, other, op):
+    def _richcmp(self, other, op):
+        if op == 1: # <=


why are the operations 1,5,2?

Copilot

Pull Request Overview

Fixes the inability to compare MatrixExpr with Expr by introducing shared comparison logic and adjusting operator overloads, plus tests and changelog updates.

Add support for MatrixExpr <=, >=, == comparisons against Expr (and arrays) via a central helper.
Disallow == on ranged Matrix constraints (MatrixExprCons) with a clear error, and add tests.
Update error types/messages for unsupported comparisons; update CHANGELOG.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
tests/test_matrix_variable.py	Adds tests for MatrixExpr vs Expr comparisons and ranged Matrix constraints behavior.
src/pyscipopt/matrix.pxi	Refactors comparison handling via _matrixexpr_richcmp; updates MatrixExpr and MatrixExprCons rich comparisons.
src/pyscipopt/expr.pxi	Harmonizes error types/messages and removes duplicate _is_number; integrates with matrix changes.
CHANGELOG.md	Notes new capability; adjusts version header format.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-17T09:07:04Z

src/pyscipopt/matrix.pxi

+    def __le__(self, other: Union[float, int, Expr, np.ndarray, 'MatrixExpr']) -> MatrixExprCons:
+        return _matrixexpr_richcmp(self, other, 1)

-        return expr_cons_matrix.view(MatrixExprCons)
+    def __ge__(self, other: Union[float, int, Expr, np.ndarray, 'MatrixExpr']) -> MatrixExprCons:
+        return _matrixexpr_richcmp(self, other, 5)

-    def __ge__(self, other: Union[float, int, Variable, np.ndarray, 'MatrixExpr']) -> np.ndarray:
-
-        expr_cons_matrix = np.empty(self.shape, dtype=object)
-        if _is_number(other) or isinstance(other, Variable):
-            for idx in np.ndindex(self.shape):
-                expr_cons_matrix[idx] = self[idx] >= other
-
-        elif isinstance(other, np.ndarray):
-            for idx in np.ndindex(self.shape):
-                expr_cons_matrix[idx] = self[idx] >= other[idx]    
-        else:
-            raise TypeError(f"Unsupported type {type(other)}")
+    def __eq__(self, other: Union[float, int, Expr, np.ndarray, 'MatrixExpr']) -> MatrixExprCons:
+        return _matrixexpr_richcmp(self, other, 2)


Forward references in type annotations to Expr and MatrixExprCons can raise NameError at import time because these names are defined later in the include order. Quote the forward-referenced types (and the return type) to avoid runtime evaluation, or remove/relax the annotations. For example: def le(..., other: Union[float, int, 'Expr', np.ndarray, 'MatrixExpr']) -> 'MatrixExprCons'.

Copilot · 2025-10-17T09:07:04Z

src/pyscipopt/matrix.pxi

+    res = np.empty(self.shape, dtype=object)
+    if _is_number(other) or isinstance(other, Expr):
+        for idx in np.ndindex(self.shape):
+            res[idx] = _richcmp(self[idx], other, op)
+
+    elif isinstance(other, np.ndarray):
+        for idx in np.ndindex(self.shape):
+            res[idx] = _richcmp(self[idx], other[idx], op)
+
+    else:
+        raise TypeError(f"Unsupported type {type(other)}")
+
+    return res.view(MatrixExprCons)


[nitpick] This change removes support for comparing MatrixExpr with Variable (previously handled via isinstance(other, Variable)). That is a breaking change and will now raise TypeError for x <= var. To preserve backward compatibility while adding Expr support, extend the check to include Variable (and optionally GenExpr), e.g.: if _is_number(other) or isinstance(other, (Expr, Variable, GenExpr)):. Also update the signatures accordingly (using quoted annotations to avoid forward-ref issues).

Copilot · 2025-10-17T09:07:05Z

CHANGELOG.md

 ### Added
 ### Fixed
 ### Changed
+- MatrixVariable supported to compare with Expr


Improve grammar: 'MatrixVariable supports comparing with Expr' or 'Add support for comparing MatrixVariable with Expr'.

Suggested change

- MatrixVariable supported to compare with Expr

- MatrixVariable now supports comparison with Expr

Copilot · 2025-10-17T09:07:05Z

CHANGELOG.md

 ### Removed

-## v5.6.0 - 2025.08.26
+## 5.6.0 - 2025.08.26


[nitpick] Version header format is inconsistent with prior entries ('## v5.x.x'). For consistency, consider reverting to '## v5.6.0 - 2025.08.26'.

Suggested change

## 5.6.0 - 2025.08.26

## v5.6.0 - 2025.08.26

Joao-Dionisio · 2025-10-17T09:09:11Z

tests/test_matrix_variable.py

+
+    # test "<=" and ">=" operator
+    x = m.addMatrixVar(3)
+    m.addMatrixCons((x <= 1) >= 1)


What does this mean? that at least one entry in matrix variable x is at most 1? I find it confusing. If so, why shouldn't the above also work?

Zeroto521 added 12 commits September 4, 2025 16:54

Replace Variable with Expr in MatrixExpr

d33379c

Updated type hints and isinstance checks in MatrixExpr comparison methods to use Expr instead of Variable. This change improves compatibility with broader expression types in matrix operations.

add test case

6c48a73

Replace Variable with Expr in MatrixExprCons

b578ea5

Updated type hints and isinstance checks in MatrixExprCons.__le__ and __ge__ methods to use Expr instead of Variable. This change improves consistency with the expected types for matrix expression constraints.

add test case

e8db5a1

Update CHANGELOG.md

aae9df9

Add test for ranged matrix constraint

d8a9377

Introduces test_ranged_matrix_cons to verify correct behavior when adding a ranged matrix constraint to the model. Ensures that the matrix variable x is set to ones as expected.

Replace TypeError with NotImplementedError in __eq__

a09be1a

The __eq__ method of MatrixExprCons now raises NotImplementedError with a descriptive message instead of TypeError, clarifying that '==' comparison is not supported.

Add tests for matrix constraint operators

771437b

Added tests for '<=', '>=', and '==' operators in matrix constraints. Verified correct exception is raised for unsupported '==' operator.

Update CHANGELOG.md

2b9a3c0

BUG: fix circular imports

b7b1321

Relocated the _is_number utility from expr.pxi to matrix.pxi for better modularity. Updated _matrixexpr_richcmp to use a local _richcmp helper for comparison operations.

Fix matrix comparison shape handling

987c219

Replaces usage of undefined 'shape' variable with 'self.shape' when creating the result array in _matrixexpr_richcmp, ensuring correct array dimensions.

Zeroto521 commented Sep 4, 2025

View reviewed changes

Zeroto521 added 3 commits September 4, 2025 18:33

Fix redundant .all() calls in matrix variable tests

7a1275d

Removed unnecessary double .all() calls in assertions for matrix variable tests, simplifying the checks for equality with np.ones(3).

Fix matrix variable test assertions to use getVal

f1dc2fa

Updated assertions in test_matrix_variable.py to use m.getVal(x) and m.getVal(y) instead of direct variable comparison. This ensures the tests check the evaluated values from the model rather than the symbolic variables.

let MatrixExprCons support <= and >= operator

b6dcf42

Zeroto521 commented Sep 4, 2025

View reviewed changes

Zeroto521 added 8 commits September 4, 2025 19:56

Refactor matrix comparison tests to optimize assertions and remove re…

64ae70e

…dundant checks

let MatrixExprCons support <= and >= operator

f69ce7e

find what type it is

3700261

align with __add__

c677b34

test "==" first

bca7262

Revert "let MatrixExprCons support <= and >= operator"

cb600b2

This reverts commit f69ce7e.

Revert "let MatrixExprCons support <= and >= operator"

a3a6239

This reverts commit b6dcf42.

find what type it is

06f8ebc

Zeroto521 force-pushed the fix/1061 branch from a0c4995 to 06f8ebc Compare September 4, 2025 13:20

test expr

ef5aecf

Zeroto521 force-pushed the fix/1061 branch from abd94f9 to ef5aecf Compare September 4, 2025 13:33

Change the order

ceaab05

Zeroto521 added 4 commits September 4, 2025 21:41

Remove ExprCons

3861420

Can't add with ExprCons

Ranged ExprCons requires number

a2ae9c9

Update CHANGELOG.md

6afa150

Lint codes with 4 spaces

88a935f

Zeroto521 commented Sep 4, 2025

View reviewed changes

Zeroto521 force-pushed the fix/1061 branch from 1e3665f to 88a935f Compare September 4, 2025 15:21

Merge branch 'master' into fix/1061

d3b0fff

Joao-Dionisio requested a review from Copilot October 17, 2025 09:03

Joao-Dionisio reviewed Oct 17, 2025

View reviewed changes

Copilot AI reviewed Oct 17, 2025

View reviewed changes

Joao-Dionisio reviewed Oct 17, 2025

View reviewed changes

		if not _is_number(other) or not isinstance(other, MatrixExpr):
		raise TypeError('Ranged MatrixExprCons is not well defined!')

	def __richcmp__(self, other, op):
	'''turn it into a constraint'''
	if op == 1: # <=
	if not self._rhs is None:
	raise TypeError('ExprCons already has upper bound')
	assert not self._lhs is None

	if not _is_number(other):
	raise TypeError('Ranged ExprCons is not well defined!')

	return ExprCons(self.expr, lhs=self._lhs, rhs=float(other))
	elif op == 5: # >=
	if not self._lhs is None:
	raise TypeError('ExprCons already has lower bound')
	assert self._lhs is None
	assert not self._rhs is None

	if not _is_number(other):
	raise TypeError('Ranged ExprCons is not well defined!')

	return ExprCons(self.expr, lhs=float(other), rhs=self._rhs)
	else:
	raise TypeError

	- MatrixVariable supported to compare with Expr
	- MatrixVariable now supports comparison with Expr

BUG: MatrixExpr can't be compared with Expr #1069

Are you sure you want to change the base?

BUG: MatrixExpr can't be compared with Expr #1069

Conversation

Zeroto521 commented Sep 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanberthias commented Sep 16, 2025

Uh oh!

Zeroto521 commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Zeroto521 commented Sep 17, 2025 •

edited

Loading