Skip to content

BUG: _VarArray can't handle MatrixVar #1044

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.

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 17 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
### Fixed
- Raised an error when an expression is used when a variable is required
- Fixed some compile warnings
- _VarArray can accept MatrixVariable now
Comment on lines 17 to +20
Copy link
Contributor

@DominikKamp DominikKamp Aug 5, 2025

Choose a reason for hiding this comment

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

Suggested change
### Fixed
- Raised an error when an expression is used when a variable is required
- Fixed some compile warnings
- _VarArray can accept MatrixVariable now
- addConsIndicator() accepts MatrixVariable as binvar
### Fixed
- Raised an error when an expression is used when a variable is required
- Fixed some compile warnings

As far as I know, indicator constraints do not support matrix variables yet, which would require some documentation of this addition function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are many methods like addConsSOS1, addConsSOS2, addConsAnd, and more that use _VarArray
This pr doesn't work for addConsIndicator

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, can we then add a passing test and mention in the changelog what works now?

### Changed
- MatrixExpr.sum() now supports axis arguments and can return either a scalar or MatrixExpr, depending on the result dimensions.
- AddMatrixCons() also accepts ExprCons.
Expand Down
25 changes: 11 additions & 14 deletions src/pyscipopt/scip.pxi
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@
if rc == SCIP_OKAY:
pass
elif rc == SCIP_ERROR:
raise Exception('SCIP: unspecified error!')

Check failure on line 311 in src/pyscipopt/scip.pxi

View workflow job for this annotation

GitHub Actions / test-coverage (3.11)

SCIP: unspecified error!
elif rc == SCIP_NOMEMORY:
raise MemoryError('SCIP: insufficient memory error!')
elif rc == SCIP_READERROR:
Expand Down Expand Up @@ -2468,21 +2468,18 @@

def __cinit__(self, object vars):
if isinstance(vars, Variable):
self.size = 1
self.ptr = <SCIP_VAR**> malloc(sizeof(SCIP_VAR*))
self.ptr[0] = (<Variable>vars).scip_var
vars = [vars]
elif isinstance(vars, (list, tuple, MatrixVariable)):
vars = np.ravel(vars)
else:
if not isinstance(vars, (list, tuple)):
raise TypeError("Expected Variable or list of Variable, got %s." % type(vars))
self.size = len(vars)
if self.size == 0:
self.ptr = NULL
else:
self.ptr = <SCIP_VAR**> malloc(self.size * sizeof(SCIP_VAR*))
for i, var in enumerate(vars):
if not isinstance(var, Variable):
raise TypeError("Expected Variable, got %s." % type(var))
self.ptr[i] = (<Variable>var).scip_var
raise TypeError(f"Expected Variable or list of Variable, got {type(vars)}.")

self.size = len(vars)
self.ptr = <SCIP_VAR**> malloc(self.size * sizeof(SCIP_VAR*)) if self.size else NULL
for i, var in enumerate(vars):
if not isinstance(var, Variable):
raise TypeError(f"Expected Variable, got {type(var)}.")
self.ptr[i] = (<Variable>var).scip_var

def __dealloc__(self):
if self.ptr != NULL:
Expand Down
34 changes: 34 additions & 0 deletions tests/test_cons.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,40 @@ def test_cons_indicator():
assert m.isEQ(m.getVal(x), 1)
assert c1.getConshdlrName() == "indicator"

def test_cons_indicator_with_matrix_binvar():
# test matrix variable binvar #1043
m = Model()

with pytest.raises(TypeError):
m.addConsIndicator(m.addVar(vtype="B") <= 1, 1)

# test binvar with (1, 1, 1) shape of matrix variable
x = m.addVar(vtype="B")
binvar1 = m.addMatrixVar(((1, 1, 1)), vtype="B")
m.addConsIndicator(x >= 1, binvar1, activeone=True)
m.addConsIndicator(x <= 0, binvar1, activeone=False)

# test binvar with (2, 3) shape of matrix variable
y = m.addVar(vtype="B")
binvar2 = m.addMatrixVar(((2, 3)), vtype="B")
m.addConsIndicator(y >= 1, binvar2, activeone=True)
m.addConsIndicator(y <= 0, binvar2, activeone=False)

# test binvar with (2, 1) shape of list of lists
z = m.addVar(vtype="B")
binvar3 = [[m.addVar(vtype="B")], [m.addVar(vtype="B")]]
m.addConsIndicator(z >= 1, binvar3, activeone=True)
m.addConsIndicator(z <= 0, binvar3, activeone=False)
Comment on lines +190 to +205
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there somewhere documented when addConsIndicator() considers a matrix binary variable active?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

binvar1 and binvar2 both are matrix binvary

Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused. We add a test for addConsIndicator() although it does not work according to https://github.com/scipopt/PySCIPOpt/pull/1044/files#r2255663332? But it should pass in this form, so the description of addConsIndicator() needs to be extended to explain how a matrix binvar is handled. Are some entries or all entries required to be one so that the indicator constraint considers it active if activeone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't directly access _VarArray, so it has to test _VarArray indirectly via addConsIndicator.
If addConsIndicator works fine with a matrix binvar. _VarArray works fine with a matrix too.

Copy link
Contributor

Choose a reason for hiding this comment

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

But addConsIndicator() does not work fine with a matrix variable since it only picks the first entry. The bug here rather seems to be that addConsIndicator() does not ensure that binvar is a variable. If this translation of a matrix variable into _VarArray is helpful somewhere, then it should be applied properly there and put on the list of added features, generalizing _VarArray alone without usecase is not enough, so maybe this should be combined with another change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know if any other methods would use all the results of _VarArray? So we could test all elements.
I only met this case in my work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Constraint addition methods, which get a list of variables like you mentioned in #1044 (comment), but for these I doubt that the ravelling is expected. So _VarArray is not designed for matrix variables, however, one could think about generalizing it to maintain the matrix structure, and if we then want to generalize all addition methods to matrix variables, we would need documented definitions how they are handled respectively. To fix a bug, adding an assertion into _VarArray that the given structure is not a list of lists of variables or a matrix variable and throwing an exception in addConsIndicator() if those are provided is the way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Joao-Dionisio what do you think about this. In my opinion, matrix is also a list. But _VarArray can’t handle matrix, so it’s a bug for _VarArray. This pr is done for that. So for addConsIndicator binvar type problem should be done in another pr not this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

_VarArray should not need to handle a proper matrix because addConsIndicator() currently does not support it. Letting _VarArray support one-dimensional matrices would be possible. But just calling ravel is too general for this, so there should be an exception if multiple dimensions are present.


m.setObjective(
binvar1.sum() + binvar2.sum() + binvar3[0][0] + binvar3[1][0], "maximize"
)
m.optimize()

assert m.getVal(x) == 1
assert m.getVal(y) == 1
assert m.getVal(z) == 1

@pytest.mark.xfail(
reason="addConsIndicator doesn't behave as expected when binary variable is False. See Issue #717."
)
Expand Down