BUG: _VarArray can't handle MatrixVar
#1044
Conversation
Simplifies and clarifies the logic for initializing _VarArray by restructuring type checks and error handling. Now uses f-strings for error messages and ensures consistent handling of empty lists and invalid types.
Updated the _VarArray class constructor to handle MatrixVariable inputs in addition to Variable, list, and tuple. Refactored pointer allocation logic into a helper function for improved code clarity and maintainability.
Introduces a new test to verify that addConsIndicator works correctly when the binary variable is a matrix variable, addressing issue scipopt#1043.
To fix "Storing unsafe C derivative of temporary Python reference"
The create_ptr function in _VarArray.__cinit__ no longer takes a separate size argument and instead uses len(vars) directly. This simplifies the function signature and reduces redundancy.
Inlined the pointer creation logic in _VarArray's __cinit__ method, removing the inner create_ptr function. This simplifies the code and improves readability while maintaining the same functionality.
The test now checks addConsIndicator with both activeone=True and activeone=False, and updates the objective to use binvar.sum().
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1044 +/- ##
==========================================
+ Coverage 54.13% 54.32% +0.19%
==========================================
Files 22 23 +1
Lines 5045 5257 +212
==========================================
+ Hits 2731 2856 +125
- Misses 2314 2401 +87 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ### Fixed | ||
| - Raised an error when an expression is used when a variable is required | ||
| - Fixed some compile warnings | ||
| - _VarArray can accept MatrixVariable now |
There was a problem hiding this comment.
| ### 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.
There was a problem hiding this comment.
There are many methods like addConsSOS1, addConsSOS2, addConsAnd, and more that use _VarArray
This pr doesn't work for addConsIndicator
There was a problem hiding this comment.
Okay, can we then add a passing test and mention in the changelog what works now?
| 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) |
There was a problem hiding this comment.
Is there somewhere documented when addConsIndicator() considers a matrix binary variable active?
There was a problem hiding this comment.
binvar1 and binvar2 both are matrix binvary
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
_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.
fix #1043