Skip to content

Fix binary operations between GenExpr and MatrixExpr #1071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+33 −2
Open

Fix binary operations between GenExpr and MatrixExpr #1071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3394aa7
Add tests for all binary operations
jonathanberthias Sep 13, 2025
717ab15
Fix binops in all cases
jonathanberthias Sep 13, 2025
aad010c
Merge branch 'master' into fix-binops
Joao-Dionisio Oct 9, 2025
2d1c15e
Merge branch 'master' into fix-binops
Joao-Dionisio Oct 9, 2025
c7025da
Add changelog entry
jonathanberthias Oct 9, 2025
80cb82f
Fix model for tests must be at module level
jonathanberthias Oct 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## Unreleased
### Added
### Fixed
- Implemented all binary operations between MatrixExpr and GenExpr
### Changed
### Removed

Expand Down
13 changes: 11 additions & 2 deletions src/pyscipopt/expr.pxi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ def buildGenExprObj(expr):
GenExprs = np.empty(expr.shape, dtype=object)
for idx in np.ndindex(expr.shape):
GenExprs[idx] = buildGenExprObj(expr[idx])
return GenExprs
return GenExprs.view(MatrixExpr)

else:
assert isinstance(expr, GenExpr)
Expand Down Expand Up @@ -223,6 +223,9 @@ cdef class Expr:
return self

def __mul__(self, other):
if isinstance(other, MatrixExpr):
return other * self

if _is_number(other):
f = float(other)
return Expr({v:f*c for v,c in self.terms.items()})
Expand Down Expand Up @@ -420,6 +423,9 @@ cdef class GenExpr:
return UnaryExpr(Operator.fabs, self)

def __add__(self, other):
if isinstance(other, MatrixExpr):
return other + self

left = buildGenExprObj(self)
right = buildGenExprObj(other)
ans = SumExpr()
Expand Down Expand Up @@ -475,6 +481,9 @@ cdef class GenExpr:
# return self

def __mul__(self, other):
if isinstance(other, MatrixExpr):
return other * self

left = buildGenExprObj(self)
right = buildGenExprObj(other)
ans = ProdExpr()
Expand Down Expand Up @@ -537,7 +546,7 @@ cdef class GenExpr:
def __truediv__(self,other):
divisor = buildGenExprObj(other)
# we can't divide by 0
if divisor.getOp() == Operator.const and divisor.number == 0.0:
if isinstance(divisor, GenExpr) and divisor.getOp() == Operator.const and divisor.number == 0.0:
raise ZeroDivisionError("cannot divide by 0")
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The division by zero check is incomplete. If divisor is not a GenExpr (e.g., a MatrixExpr), the check is bypassed and division by zero could still occur in the divisor**(-1) operation.

Suggested change
raise ZeroDivisionError("cannot divide by 0")
raise ZeroDivisionError("cannot divide by 0")
elif isinstance(divisor, MatrixExpr):
# Check if MatrixExpr is a zero matrix or scalar zero
if hasattr(divisor, "is_zero") and divisor.is_zero():
raise ZeroDivisionError("cannot divide by zero MatrixExpr")
# If MatrixExpr has a 'number' attribute and is scalar
if hasattr(divisor, "number") and divisor.number == 0.0:
raise ZeroDivisionError("cannot divide by 0")

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried with 0 as the divisor and every time I end up with a ZeroDivisionError, not a crash. It could be nice to add a test that it is always the case but it seems unrelated to this PR. What do you think @Joao-Dionisio?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or actually, we could remove the check for division by 0 entirely from this function, and let a downstream function raise the error. All tests pass if I do so.

return self * divisor**(-1)

Expand Down
21 changes: 21 additions & 0 deletions tests/test_matrix_variable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import operator
import pdb
import pprint
import pytest
from pyscipopt import Model, Variable, log, exp, cos, sin, sqrt
from pyscipopt import Expr, MatrixExpr, MatrixVariable, MatrixExprCons, MatrixConstraint, ExprCons
from pyscipopt.scip import GenExpr
from time import time

import numpy as np
Expand Down Expand Up @@ -394,6 +396,25 @@ def test_matrix_cons_indicator():
assert (m.getVal(x) == np.array([[5, 5, 5], [5, 5, 5]])).all().all()


_binop_model = Model()

def var():
return _binop_model.addVar()

def genexpr():
return _binop_model.addVar() ** 0.6

def matvar():
return _binop_model.addMatrixVar((1,))

@pytest.mark.parametrize("right", [var(), genexpr(), matvar()], ids=["var", "genexpr", "matvar"])
@pytest.mark.parametrize("left", [var(), genexpr(), matvar()], ids=["var", "genexpr", "matvar"])
@pytest.mark.parametrize("op", [operator.add, operator.sub, operator.mul, operator.truediv])
def test_binop(op, left, right):
res = op(left, right)
assert isinstance(res, (Expr, GenExpr, MatrixExpr))


def test_matrix_matmul_return_type():
# test #1058, require returning type is MatrixExpr not MatrixVariable
m = Model()
Expand Down
Loading