From d172724105451e4b5ce28b3443cc70e2b9fbbbf5 Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Wed, 28 May 2025 13:27:48 +0200 Subject: [PATCH 01/21] Fix 1004 --- CHANGELOG.md | 1 + src/pyscipopt/scip.pxi | 11 ++++++++++- tests/test_cons.py | 19 +++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index efd8ed922..881c665d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Added isPositive(), isNegative(), isFeasLE(), isFeasLT(), isFeasGE(), isFeasGT(), isHugeValue(), and tests - Added SCIP_LOCKTYPE, addVarLocksType(), getNLocksDown(), getNLocksUp(), getNLocksDownType(), getNLocksUpType(), and tests ### Fixed +- Fixed segmentation fault when logical constraints were used with expressions rather than variables. ### Changed ### Removed diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index 53bed228f..192dbd4a4 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -6308,6 +6308,9 @@ cdef class Model: _resvar = (resvar).scip_var for i, var in enumerate(vars): + if not isinstance(var, Variable): + raise TypeError("Expected Variable, got %s." % type(var)) + _vars[i] = (var).scip_var if name == '': @@ -6373,6 +6376,9 @@ cdef class Model: _resvar = (resvar).scip_var for i, var in enumerate(vars): + if not isinstance(var, Variable): + raise TypeError("Expected Variable, got %s." % type(var)) + _vars[i] = (var).scip_var if name == '': @@ -6437,6 +6443,9 @@ cdef class Model: assert type(rhsvar) is type(bool()), "Provide BOOLEAN value as rhsvar, you gave %s." % type(rhsvar) for i, var in enumerate(vars): + if not isinstance(var, Variable): + raise TypeError("Expected Variable, got %s." % type(var)) + _vars[i] = (var).scip_var if name == '': @@ -8763,7 +8772,7 @@ cdef class Model: return Node.create(downchild), Node.create(eqchild), Node.create(upchild) - def branchVarVal(self, variable, value): + def branchVarVal(self, Variable variable, value): """ Branches on variable using a value which separates the domain of the variable. diff --git a/tests/test_cons.py b/tests/test_cons.py index a3c663ee0..527265126 100644 --- a/tests/test_cons.py +++ b/tests/test_cons.py @@ -72,6 +72,25 @@ def test_cons_logical(): assert m.isEQ(m.getVal(result1), 1) assert m.isEQ(m.getVal(result2), 0) +@pytest.mark.xfail( + reason="Logical constraints expect lists of binary variables" +) +def test_cons_logical_fail(): + m = Model() + x1 = m.addVar(vtype="B") + x2 = m.addVar(vtype="B") + x3 = m.addVar(vtype="B") + x4 = m.addVar(vtype="B") + result1 = m.addVar(vtype="B") + + m.addCons(x3 == 1 - x1) + m.addCons(x4 == 1 - x2) + + # result1 true + m.addConsOr([x1*x3, x2*x4], result1) + + m.optimize() + def test_SOScons(): m = Model() x = {} From baf646564bed4cc53f70857c71fa59b93c918745 Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Wed, 28 May 2025 14:50:08 +0200 Subject: [PATCH 02/21] refactor variable conversion --- src/pyscipopt/scip.pxi | 33 +++++++++++++++++++++++++-------- tests/test_cons.py | 4 +--- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index 192dbd4a4..b482aff1d 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -2397,6 +2397,18 @@ cdef void relayErrorMessage(void *messagehdlr, FILE *file, const char *msg) noex fputs(msg, file) fflush(file) +cdef class VarArrayWrapper: + cdef SCIP_VAR** ptr + cdef int size + + def __cinit__(self, int size): + self.size = size + self.ptr = malloc(size * sizeof(SCIP_VAR*)) + + def __dealloc__(self): + if self.ptr != NULL: + free(self.ptr) + # - remove create(), includeDefaultPlugins(), createProbBasic() methods # - replace free() by "destructor" # - interface SCIPfreeProb() @@ -6327,6 +6339,17 @@ cdef class Model: return pyCons + def _convert_var_to_scipvar(self, vars, nvars): + cdef VarArrayWrapper wrapper = VarArrayWrapper(nvars) + + for i, var in enumerate(vars): + if not isinstance(var, Variable): + raise TypeError("Expected Variable, got %s." % type(var)) + + wrapper.ptr[i] = (var).scip_var + + return wrapper + def addConsOr(self, vars, resvar, name="", initial=True, separate=True, enforce=True, check=True, propagate=True, local=False, modifiable=False, dynamic=False, @@ -6369,17 +6392,13 @@ cdef class Model: """ cdef int nvars = len(vars) - cdef SCIP_VAR** _vars = malloc(nvars * sizeof(SCIP_VAR*)) cdef SCIP_VAR* _resvar cdef SCIP_CONS* scip_cons cdef int i _resvar = (resvar).scip_var - for i, var in enumerate(vars): - if not isinstance(var, Variable): - raise TypeError("Expected Variable, got %s." % type(var)) - - _vars[i] = (var).scip_var + cdef VarArrayWrapper wrapper = self._convert_var_to_scipvar(vars, nvars) + cdef SCIP_VAR** _vars = wrapper.ptr if name == '': name = 'c'+str(SCIPgetNConss(self._scip)+1) @@ -6391,8 +6410,6 @@ cdef class Model: pyCons = Constraint.create(scip_cons) PY_SCIP_CALL(SCIPreleaseCons(self._scip, &scip_cons)) - free(_vars) - return pyCons def addConsXor(self, vars, rhsvar, name="", diff --git a/tests/test_cons.py b/tests/test_cons.py index 527265126..c10524439 100644 --- a/tests/test_cons.py +++ b/tests/test_cons.py @@ -72,9 +72,7 @@ def test_cons_logical(): assert m.isEQ(m.getVal(result1), 1) assert m.isEQ(m.getVal(result2), 0) -@pytest.mark.xfail( - reason="Logical constraints expect lists of binary variables" -) +@pytest.mark.xfail() def test_cons_logical_fail(): m = Model() x1 = m.addVar(vtype="B") From fe30f1a918678d3fdc6fa04eac930baaa16b5fae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Dion=C3=ADsio?= <57299939+Joao-Dionisio@users.noreply.github.com> Date: Wed, 28 May 2025 13:53:53 +0100 Subject: [PATCH 03/21] Update tests/test_cons.py Co-authored-by: DominikKamp <130753997+DominikKamp@users.noreply.github.com> --- tests/test_cons.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_cons.py b/tests/test_cons.py index c10524439..6c77efffa 100644 --- a/tests/test_cons.py +++ b/tests/test_cons.py @@ -84,7 +84,7 @@ def test_cons_logical_fail(): m.addCons(x3 == 1 - x1) m.addCons(x4 == 1 - x2) - # result1 true + # result1 false m.addConsOr([x1*x3, x2*x4], result1) m.optimize() From 928f2e504adde88b9f87a7ce6d239d853a6da4e6 Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Wed, 28 May 2025 16:18:17 +0200 Subject: [PATCH 04/21] Put function inside class --- src/pyscipopt/scip.pxi | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index b482aff1d..6b42e872d 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -2401,10 +2401,16 @@ cdef class VarArrayWrapper: cdef SCIP_VAR** ptr cdef int size - def __cinit__(self, int size): - self.size = size + def __cinit__(self, object vars): + cdef int size = len(vars) self.ptr = malloc(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] = (var).scip_var + def __dealloc__(self): if self.ptr != NULL: free(self.ptr) @@ -6397,7 +6403,7 @@ cdef class Model: cdef int i _resvar = (resvar).scip_var - cdef VarArrayWrapper wrapper = self._convert_var_to_scipvar(vars, nvars) + cdef VarArrayWrapper wrapper = VarArrayWrapper(vars) cdef SCIP_VAR** _vars = wrapper.ptr if name == '': From b4e90659b867a33d78fdfb64f6d8366522a34b4c Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Wed, 28 May 2025 17:22:31 +0200 Subject: [PATCH 05/21] Call wrapper class in a few more places --- CHANGELOG.md | 1 + src/pyscipopt/scip.pxi | 44 +++++++++++++++++++++--------------------- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 881c665d5..9ca93ca04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ - Added support for knapsack constraints - Added isPositive(), isNegative(), isFeasLE(), isFeasLT(), isFeasGE(), isFeasGT(), isHugeValue(), and tests - Added SCIP_LOCKTYPE, addVarLocksType(), getNLocksDown(), getNLocksUp(), getNLocksDownType(), getNLocksUpType(), and tests +- Added internal class _VarArrayWrapper to get the scip variables from PySCIPOpt variables ### Fixed - Fixed segmentation fault when logical constraints were used with expressions rather than variables. ### Changed diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index 6b42e872d..b5ae792b1 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -2397,7 +2397,7 @@ cdef void relayErrorMessage(void *messagehdlr, FILE *file, const char *msg) noex fputs(msg, file) fflush(file) -cdef class VarArrayWrapper: +cdef class _VarArrayWrapper: cdef SCIP_VAR** ptr cdef int size @@ -2405,11 +2405,17 @@ cdef class VarArrayWrapper: cdef int size = len(vars) self.ptr = malloc(size * sizeof(SCIP_VAR*)) - for i, var in enumerate(vars): - if not isinstance(var, Variable): - raise TypeError("Expected Variable, got %s." % type(var)) + if size == 1: + if not isinstance(vars, Variable): + raise TypeError("Expected Variable, got %s." % type(vars)) + else: + self.ptr[0] = (vars).scip_var + else: + for i, var in enumerate(vars): + if not isinstance(var, Variable): + raise TypeError("Expected Variable, got %s." % type(var)) - self.ptr[i] = (var).scip_var + self.ptr[i] = (var).scip_var def __dealloc__(self): if self.ptr != NULL: @@ -6345,17 +6351,6 @@ cdef class Model: return pyCons - def _convert_var_to_scipvar(self, vars, nvars): - cdef VarArrayWrapper wrapper = VarArrayWrapper(nvars) - - for i, var in enumerate(vars): - if not isinstance(var, Variable): - raise TypeError("Expected Variable, got %s." % type(var)) - - wrapper.ptr[i] = (var).scip_var - - return wrapper - def addConsOr(self, vars, resvar, name="", initial=True, separate=True, enforce=True, check=True, propagate=True, local=False, modifiable=False, dynamic=False, @@ -6398,13 +6393,17 @@ cdef class Model: """ cdef int nvars = len(vars) + cdef SCIP_VAR** _vars cdef SCIP_VAR* _resvar cdef SCIP_CONS* scip_cons cdef int i + cdef _VarArrayWrapper wrapper - _resvar = (resvar).scip_var - cdef VarArrayWrapper wrapper = VarArrayWrapper(vars) - cdef SCIP_VAR** _vars = wrapper.ptr + wrapper = _VarArrayWrapper(resvar) + _resvar = wrapper.ptr + + wrapper = _VarArrayWrapper(vars) + _vars = wrapper.ptr if name == '': name = 'c'+str(SCIPgetNConss(self._scip)+1) @@ -8271,9 +8270,10 @@ cdef class Model: cdef SCIP_HEUR* _heur cdef SCIP_Bool success cdef int i - - for i, var in enumerate(sub_model.getVars()): - vars[i] = (var).scip_var + cdef _VarArrayWrapper wrapper + + wrapper = _VarArrayWrapper(sub_model.getVars()) + vars = wrapper.ptr name = str_conversion(heur.name) _heur = SCIPfindHeur(self._scip, name) From 65e023252d7d57a650b70fe6ab54904d6b3637af Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Wed, 28 May 2025 17:41:01 +0200 Subject: [PATCH 06/21] Single variable --- src/pyscipopt/scip.pxi | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index b5ae792b1..f0f88428c 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -2402,15 +2402,18 @@ cdef class _VarArrayWrapper: cdef int size def __cinit__(self, object vars): - cdef int size = len(vars) - self.ptr = malloc(size * sizeof(SCIP_VAR*)) + cdef int size - if size == 1: - if not isinstance(vars, Variable): - raise TypeError("Expected Variable, got %s." % type(vars)) - else: - self.ptr[0] = (vars).scip_var + if isinstance(vars, Variable): + self.ptr = malloc(sizeof(SCIP_VAR*)) + self.ptr[0] = (vars).scip_var else: + if not isinstance(vars, (list, tuple)): + raise TypeError("Expected Variable or list of Variable, got %s." % type(vars)) + + size = len(vars) + self.ptr = malloc(size * sizeof(SCIP_VAR*)) + for i, var in enumerate(vars): if not isinstance(var, Variable): raise TypeError("Expected Variable, got %s." % type(var)) @@ -6400,7 +6403,7 @@ cdef class Model: cdef _VarArrayWrapper wrapper wrapper = _VarArrayWrapper(resvar) - _resvar = wrapper.ptr + _resvar = wrapper.ptr[0] wrapper = _VarArrayWrapper(vars) _vars = wrapper.ptr From aaec54a9499919fe59c7d408de11a1e50458abe1 Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Wed, 28 May 2025 17:43:00 +0200 Subject: [PATCH 07/21] change name to VarArray --- CHANGELOG.md | 2 +- src/pyscipopt/scip.pxi | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ca93ca04..ff9d7d5fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ - Added support for knapsack constraints - Added isPositive(), isNegative(), isFeasLE(), isFeasLT(), isFeasGE(), isFeasGT(), isHugeValue(), and tests - Added SCIP_LOCKTYPE, addVarLocksType(), getNLocksDown(), getNLocksUp(), getNLocksDownType(), getNLocksUpType(), and tests -- Added internal class _VarArrayWrapper to get the scip variables from PySCIPOpt variables +- Added internal class _VarArray to get the scip variables from PySCIPOpt variables ### Fixed - Fixed segmentation fault when logical constraints were used with expressions rather than variables. ### Changed diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index f0f88428c..4c1586e4e 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -2397,7 +2397,7 @@ cdef void relayErrorMessage(void *messagehdlr, FILE *file, const char *msg) noex fputs(msg, file) fflush(file) -cdef class _VarArrayWrapper: +cdef class _VarArray: cdef SCIP_VAR** ptr cdef int size @@ -6400,12 +6400,12 @@ cdef class Model: cdef SCIP_VAR* _resvar cdef SCIP_CONS* scip_cons cdef int i - cdef _VarArrayWrapper wrapper + cdef _VarArray wrapper - wrapper = _VarArrayWrapper(resvar) + wrapper = _VarArray(resvar) _resvar = wrapper.ptr[0] - wrapper = _VarArrayWrapper(vars) + wrapper = _VarArray(vars) _vars = wrapper.ptr if name == '': @@ -8273,9 +8273,9 @@ cdef class Model: cdef SCIP_HEUR* _heur cdef SCIP_Bool success cdef int i - cdef _VarArrayWrapper wrapper + cdef _VarArray wrapper - wrapper = _VarArrayWrapper(sub_model.getVars()) + wrapper = _VarArray(sub_model.getVars()) vars = wrapper.ptr name = str_conversion(heur.name) From 8d00fb8e4ae9727398158cbb2fca0585dd792560 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Dion=C3=ADsio?= <57299939+Joao-Dionisio@users.noreply.github.com> Date: Wed, 28 May 2025 16:56:32 +0100 Subject: [PATCH 08/21] Apply suggestions from code review Co-authored-by: DominikKamp <130753997+DominikKamp@users.noreply.github.com> --- src/pyscipopt/scip.pxi | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index 4c1586e4e..91fa56614 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -2402,23 +2402,22 @@ cdef class _VarArray: cdef int size def __cinit__(self, object vars): - cdef int size - if isinstance(vars, Variable): - self.ptr = malloc(sizeof(SCIP_VAR*)) - self.ptr[0] = (vars).scip_var + self.size = 1 + self.ptr = malloc(sizeof(SCIP_VAR*)) + self.ptr[0] = vars.scip_var else: if not isinstance(vars, (list, tuple)): raise TypeError("Expected Variable or list of Variable, got %s." % type(vars)) - size = len(vars) - self.ptr = malloc(size * sizeof(SCIP_VAR*)) + self.size = len(vars) + self.ptr = 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] = (var).scip_var + self.ptr[i] = var.scip_var def __dealloc__(self): if self.ptr != NULL: From bcc2f8894a430b55bc5d0a2ac81e993f9a73629c Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Wed, 28 May 2025 18:07:29 +0200 Subject: [PATCH 09/21] Messed around with pointers, found bug in translatesubsol --- src/pyscipopt/scip.pxi | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index 91fa56614..59b0dfa9e 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -2403,21 +2403,23 @@ cdef class _VarArray: def __cinit__(self, object vars): if isinstance(vars, Variable): - self.size = 1 - self.ptr = malloc(sizeof(SCIP_VAR*)) - self.ptr[0] = vars.scip_var + self.ptr = malloc(sizeof(SCIP_VAR*)) + self.ptr[0] = (vars).scip_var else: if not isinstance(vars, (list, tuple)): raise TypeError("Expected Variable or list of Variable, got %s." % type(vars)) - self.size = len(vars) - self.ptr = malloc(self.size * sizeof(SCIP_VAR*)) + size = len(vars) + if size == 0: + self.ptr = NULL + else: + self.ptr = malloc(size * sizeof(SCIP_VAR*)) - for i, var in enumerate(vars): - if not isinstance(var, Variable): - raise TypeError("Expected Variable, got %s." % type(var)) + for i, var in enumerate(vars): + if not isinstance(var, Variable): + raise TypeError("Expected Variable, got %s." % type(var)) - self.ptr[i] = var.scip_var + self.ptr[i] = (var).scip_var def __dealloc__(self): if self.ptr != NULL: @@ -8266,7 +8268,7 @@ cdef class Model: solution : Solution The corresponding solution in the main model """ - cdef SCIP_VAR** vars = malloc(self.getNVars() * sizeof(SCIP_VAR*)) + cdef SCIP_VAR** vars = malloc(sub_model.getNVars() * sizeof(SCIP_VAR*)) cdef SCIP_SOL* real_sol cdef SCIP_SOL* subscip_sol = sol.sol cdef SCIP_HEUR* _heur From 4e9688e24f5472798dc79c14a742deceab4f25fd Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Wed, 28 May 2025 18:19:36 +0200 Subject: [PATCH 10/21] Fix double free --- src/pyscipopt/scip.pxi | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index 59b0dfa9e..9497dc8f3 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -2403,19 +2403,24 @@ cdef class _VarArray: def __cinit__(self, object vars): if isinstance(vars, Variable): + print("a") self.ptr = malloc(sizeof(SCIP_VAR*)) self.ptr[0] = (vars).scip_var else: + print("b") + if not isinstance(vars, (list, tuple)): raise TypeError("Expected Variable or list of Variable, got %s." % type(vars)) size = len(vars) + print(size) if size == 0: self.ptr = NULL else: self.ptr = malloc(size * sizeof(SCIP_VAR*)) for i, var in enumerate(vars): + print(i) if not isinstance(var, Variable): raise TypeError("Expected Variable, got %s." % type(var)) @@ -8283,7 +8288,6 @@ cdef class Model: _heur = SCIPfindHeur(self._scip, name) PY_SCIP_CALL( SCIPtranslateSubSol(self._scip, sub_model._scip, subscip_sol, _heur, vars, &real_sol) ) solution = Solution.create(self._scip, real_sol) - free(vars) return solution From df32050bde4a251b44123ea402891a28e80ceb02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Dion=C3=ADsio?= <57299939+Joao-Dionisio@users.noreply.github.com> Date: Wed, 28 May 2025 22:46:09 +0100 Subject: [PATCH 11/21] Apply suggestions from code review Co-authored-by: DominikKamp <130753997+DominikKamp@users.noreply.github.com> --- src/pyscipopt/scip.pxi | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index 9497dc8f3..f49bd43c8 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -2403,28 +2403,23 @@ cdef class _VarArray: def __cinit__(self, object vars): if isinstance(vars, Variable): - print("a") + self.size = 1 self.ptr = malloc(sizeof(SCIP_VAR*)) - self.ptr[0] = (vars).scip_var + self.ptr[0] = vars.scip_var else: - print("b") - if not isinstance(vars, (list, tuple)): raise TypeError("Expected Variable or list of Variable, got %s." % type(vars)) - size = len(vars) - print(size) - if size == 0: + self.size = len(vars) + if self.size == 0: self.ptr = NULL else: - self.ptr = malloc(size * sizeof(SCIP_VAR*)) + self.ptr = malloc(self.size * sizeof(SCIP_VAR*)) for i, var in enumerate(vars): - print(i) if not isinstance(var, Variable): raise TypeError("Expected Variable, got %s." % type(var)) - - self.ptr[i] = (var).scip_var + self.ptr[i] = var.scip_var def __dealloc__(self): if self.ptr != NULL: @@ -6408,9 +6403,7 @@ cdef class Model: cdef int i cdef _VarArray wrapper - wrapper = _VarArray(resvar) - _resvar = wrapper.ptr[0] - + _resvar = _VarArray(resvar).ptr[0] wrapper = _VarArray(vars) _vars = wrapper.ptr From 954c7318fcc43f6d994bd641136abeb2edd3c0b5 Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Thu, 29 May 2025 00:06:39 +0200 Subject: [PATCH 12/21] Fix most suggestions --- CHANGELOG.md | 3 +-- src/pyscipopt/scip.pxi | 13 +++++++------ tests/test_cons.py | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff9d7d5fd..eda185ae9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,9 +5,8 @@ - Added support for knapsack constraints - Added isPositive(), isNegative(), isFeasLE(), isFeasLT(), isFeasGE(), isFeasGT(), isHugeValue(), and tests - Added SCIP_LOCKTYPE, addVarLocksType(), getNLocksDown(), getNLocksUp(), getNLocksDownType(), getNLocksUpType(), and tests -- Added internal class _VarArray to get the scip variables from PySCIPOpt variables ### Fixed -- Fixed segmentation fault when logical constraints were used with expressions rather than variables. +- Raised an error when logical constraints are used with expressions rather than variables. ### Changed ### Removed diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index f49bd43c8..b908d9376 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -2405,7 +2405,7 @@ cdef class _VarArray: if isinstance(vars, Variable): self.size = 1 self.ptr = malloc(sizeof(SCIP_VAR*)) - self.ptr[0] = vars.scip_var + self.ptr[0] = (vars).scip_var else: if not isinstance(vars, (list, tuple)): raise TypeError("Expected Variable or list of Variable, got %s." % type(vars)) @@ -2419,7 +2419,7 @@ cdef class _VarArray: for i, var in enumerate(vars): if not isinstance(var, Variable): raise TypeError("Expected Variable, got %s." % type(var)) - self.ptr[i] = var.scip_var + self.ptr[i] = (var).scip_var def __dealloc__(self): if self.ptr != NULL: @@ -6401,10 +6401,10 @@ cdef class Model: cdef SCIP_VAR* _resvar cdef SCIP_CONS* scip_cons cdef int i - cdef _VarArray wrapper + cdef _VarArray wrapper = _VarArray(vars) + cdef _VarArray resvar_wrapper = _VarArray(resvar) - _resvar = _VarArray(resvar).ptr[0] - wrapper = _VarArray(vars) + _resvar = resvar_wrapper.ptr[0] _vars = wrapper.ptr if name == '': @@ -8266,7 +8266,6 @@ cdef class Model: solution : Solution The corresponding solution in the main model """ - cdef SCIP_VAR** vars = malloc(sub_model.getNVars() * sizeof(SCIP_VAR*)) cdef SCIP_SOL* real_sol cdef SCIP_SOL* subscip_sol = sol.sol cdef SCIP_HEUR* _heur @@ -8274,6 +8273,8 @@ cdef class Model: cdef int i cdef _VarArray wrapper + assert sub_model.getNVars(False) >= self.getNVars(False), "The sub_model must have at least as many variables as the main model." + wrapper = _VarArray(sub_model.getVars()) vars = wrapper.ptr diff --git a/tests/test_cons.py b/tests/test_cons.py index 6c77efffa..63f7010e8 100644 --- a/tests/test_cons.py +++ b/tests/test_cons.py @@ -72,7 +72,6 @@ def test_cons_logical(): assert m.isEQ(m.getVal(result1), 1) assert m.isEQ(m.getVal(result2), 0) -@pytest.mark.xfail() def test_cons_logical_fail(): m = Model() x1 = m.addVar(vtype="B") @@ -85,7 +84,8 @@ def test_cons_logical_fail(): m.addCons(x4 == 1 - x2) # result1 false - m.addConsOr([x1*x3, x2*x4], result1) + with pytest.raises(TypeError): + m.addConsOr([x1*x3, x2*x4], result1) m.optimize() From 659e9b6ab6740d6784b04f079e8e9dadd44aa932 Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Thu, 29 May 2025 00:40:37 +0200 Subject: [PATCH 13/21] _VarArray in everything --- src/pyscipopt/scip.pxi | 100 ++++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 47 deletions(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index b908d9376..f38c861be 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -1042,17 +1042,18 @@ cdef class Solution: # fast track for Variable cdef SCIP_Real coeff + cdef _VarArray wrapper + if isinstance(expr, Variable): self._checkStage("SCIPgetSolVal") - var = expr - return SCIPgetSolVal(self.scip, self.sol, var.scip_var) + return SCIPgetSolVal(self.scip, self.sol, _VarArray(expr).ptr[0]) return sum(self._evaluate(term)*coeff for term, coeff in expr.terms.items() if coeff != 0) def _evaluate(self, term): self._checkStage("SCIPgetSolVal") result = 1 for var in term.vartuple: - result *= SCIPgetSolVal(self.scip, self.sol, ( var).scip_var) + result *= SCIPgetSolVal(self.scip, self.sol, _VarArray(var).ptr[0]) return result def __setitem__(self, Variable var, value): @@ -3472,8 +3473,7 @@ cdef class Model: # avoid CONST term of Expr if term != CONST: assert len(term) == 1 - var = term[0] - PY_SCIP_CALL(SCIPchgVarObj(self._scip, var.scip_var, coef)) + PY_SCIP_CALL(SCIPchgVarObj(self._scip, _VarArray(term[0]).ptr[0], coef)) if sense == "minimize": self.setMinimize() @@ -5169,9 +5169,11 @@ cdef class Model: cdef SCIP_CONS* scip_cons cdef SCIP_Real coeff cdef int i + cdef _VarArray wrapper for i, (key, coeff) in enumerate(terms.items()): - vars_array[i] = (key[0]).scip_var + wrapper = _VarArray(key[0]) + vars_array[i] = wrapper.ptr[0] coeffs_array[i] = coeff PY_SCIP_CALL(SCIPcreateConsLinear( @@ -5219,15 +5221,13 @@ cdef class Model: for v, c in terms.items(): if len(v) == 1: # linear - var = v[0] - PY_SCIP_CALL(SCIPaddLinearVarNonlinear(self._scip, scip_cons, var.scip_var, c)) + PY_SCIP_CALL(SCIPaddLinearVarNonlinear(self._scip, scip_cons, _VarArray(v[0]).ptr[0], c)) else: # nonlinear assert len(v) == 2, 'term length must be 1 or 2 but it is %s' % len(v) varexprs = malloc(2 * sizeof(SCIP_EXPR*)) - var1, var2 = v[0], v[1] - PY_SCIP_CALL( SCIPcreateExprVar(self._scip, &varexprs[0], var1.scip_var, NULL, NULL) ) - PY_SCIP_CALL( SCIPcreateExprVar(self._scip, &varexprs[1], var2.scip_var, NULL, NULL) ) + PY_SCIP_CALL( SCIPcreateExprVar(self._scip, &varexprs[0], _VarArray(v[0]).ptr[0], NULL, NULL) ) + PY_SCIP_CALL( SCIPcreateExprVar(self._scip, &varexprs[1], _VarArray(v[1]).ptr[0], NULL, NULL) ) PY_SCIP_CALL( SCIPcreateExprProduct(self._scip, &prodexpr, 2, varexprs, 1.0, NULL, NULL) ) PY_SCIP_CALL( SCIPaddExprNonlinear(self._scip, scip_cons, prodexpr, c) ) @@ -5260,6 +5260,7 @@ cdef class Model: cdef SCIP_EXPR** varexprs cdef SCIP_EXPR** monomials cdef SCIP_CONS* scip_cons + cdef _VarArray wrapper cdef int* idxs cdef int i cdef int j @@ -5277,7 +5278,9 @@ cdef class Model: for i, (term, coef) in enumerate(terms.items()): termvars = malloc(len(term) * sizeof(SCIP_VAR*)) for j, var in enumerate(term): - termvars[j] = (var).scip_var + wrapper = _VarArray(var) + termvars[j] = wrapper.ptr[0] + PY_SCIP_CALL( SCIPcreateExprMonomial(self._scip, &monomials[i], len(term), termvars, NULL, NULL, NULL) ) termcoefs[i] = coef free(termvars) @@ -5331,6 +5334,7 @@ cdef class Model: cdef SCIP_EXPR** childrenexpr cdef SCIP_EXPR** scipexprs cdef SCIP_CONS* scip_cons + cdef _VarArray wrapper cdef int nchildren cdef int c cdef int i @@ -5360,8 +5364,9 @@ cdef class Model: if opidx == Operator.varidx: assert len(node[1]) == 1 pyvar = node[1][0] # for vars we store the actual var! - PY_SCIP_CALL( SCIPcreateExprVar(self._scip, &scipexprs[i], (pyvar).scip_var, NULL, NULL) ) - vars[varpos] = (pyvar).scip_var + wrapper = _VarArray(pyvar) + vars[varpos] = wrapper.ptr[0] + PY_SCIP_CALL( SCIPcreateExprVar(self._scip, &scipexprs[i], vars[varpos], NULL, NULL) ) varpos += 1 continue if opidx == Operator.const: @@ -6132,6 +6137,7 @@ cdef class Model: cdef SCIP_VAR** vars_array = malloc(nvars * sizeof(SCIP_VAR*)) cdef SCIP_Longint* weights_array = malloc(nvars * sizeof(SCIP_Real)) cdef SCIP_CONS* scip_cons + cdef _VarArray wrapper assert nvars == len(weights), "Number of variables and weights must be the same." @@ -6139,7 +6145,8 @@ cdef class Model: name = 'c'+str(SCIPgetNConss(self._scip)+1) for i in range(nvars): - vars_array[i] = (vars[i]).scip_var + wrapper = _VarArray(vars[i]) + vars_array[i] = wrapper.ptr[0] weights_array[i] = weights[i] PY_SCIP_CALL(SCIPcreateConsKnapsack( @@ -6210,13 +6217,11 @@ cdef class Model: if weights is None: for v in vars: - var = v - PY_SCIP_CALL(SCIPappendVarSOS1(self._scip, scip_cons, var.scip_var)) + PY_SCIP_CALL(SCIPappendVarSOS1(self._scip, scip_cons, _VarArray(v).ptr[0])) else: nvars = len(vars) for i in range(nvars): - var = vars[i] - PY_SCIP_CALL(SCIPaddVarSOS1(self._scip, scip_cons, var.scip_var, weights[i])) + PY_SCIP_CALL(SCIPaddVarSOS1(self._scip, scip_cons, _VarArray(vars[i]).ptr[0], weights[i])) PY_SCIP_CALL(SCIPaddCons(self._scip, scip_cons)) @@ -6275,13 +6280,11 @@ cdef class Model: if weights is None: for v in vars: - var = v - PY_SCIP_CALL(SCIPappendVarSOS2(self._scip, scip_cons, var.scip_var)) + PY_SCIP_CALL(SCIPappendVarSOS2(self._scip, scip_cons, _VarArray(v).ptr[0])) else: nvars = len(vars) for i in range(nvars): - var = vars[i] - PY_SCIP_CALL(SCIPaddVarSOS2(self._scip, scip_cons, var.scip_var, weights[i])) + PY_SCIP_CALL(SCIPaddVarSOS2(self._scip, scip_cons, _VarArray(vars[i]).ptr[0], weights[i])) PY_SCIP_CALL(SCIPaddCons(self._scip, scip_cons)) @@ -6331,15 +6334,13 @@ cdef class Model: cdef int nvars = len(vars) cdef SCIP_VAR** _vars = malloc(nvars * sizeof(SCIP_VAR*)) cdef SCIP_VAR* _resvar + cdef _VarArray resvar_wrapper = _VarArray(resvar) + cdef _VarArray vars_wrapper = _VarArray(vars) cdef SCIP_CONS* scip_cons cdef int i - _resvar = (resvar).scip_var - for i, var in enumerate(vars): - if not isinstance(var, Variable): - raise TypeError("Expected Variable, got %s." % type(var)) - - _vars[i] = (var).scip_var + _resvar = resvar_wrapper.ptr[0] + _vars = vars_wrapper.ptr if name == '': name = 'c'+str(SCIPgetNConss(self._scip)+1) @@ -6464,13 +6465,10 @@ cdef class Model: cdef SCIP_VAR** _vars = malloc(nvars * sizeof(SCIP_VAR*)) cdef SCIP_CONS* scip_cons cdef int i + cdef _VarArray wrapper = _VarArray(vars) assert type(rhsvar) is type(bool()), "Provide BOOLEAN value as rhsvar, you gave %s." % type(rhsvar) - for i, var in enumerate(vars): - if not isinstance(var, Variable): - raise TypeError("Expected Variable, got %s." % type(var)) - - _vars[i] = (var).scip_var + _vars = wrapper.ptr if name == '': name = 'c'+str(SCIPgetNConss(self._scip)+1) @@ -6537,6 +6535,9 @@ cdef class Model: cdef SCIP_VAR* indvar cdef SCIP_CONS* scip_cons cdef int i + cdef _VarArray v_wrapper + cdef _VarArray indvar_wrapper + cdef SCIP_VAR* scip_var PY_SCIP_CALL(SCIPcreateConsCardinality(self._scip, &scip_cons, str_conversion(name), 0, NULL, cardval, NULL, NULL, initial, separate, enforce, check, propagate, local, dynamic, removable, stickingatnode)) @@ -6549,15 +6550,18 @@ cdef class Model: name = 'c'+str(SCIPgetNConss(self._scip)+1) for i, v in enumerate(consvars): - var = v + v_wrapper = _VarArray(v) + scip_var = v_wrapper.ptr[0] if indvars: - indvar = (indvars[i]).scip_var + indvar_wrapper = _VarArray(indvars[i]) + indvar = indvar_wrapper.ptr[0] else: indvar = NULL + if weights is None: - PY_SCIP_CALL(SCIPappendVarCardinality(self._scip, scip_cons, var.scip_var, indvar)) + PY_SCIP_CALL(SCIPappendVarCardinality(self._scip, scip_cons, scip_var, indvar)) else: - PY_SCIP_CALL(SCIPaddVarCardinality(self._scip, scip_cons, var.scip_var, indvar, weights[i])) + PY_SCIP_CALL(SCIPaddVarCardinality(self._scip, scip_cons, scip_var, indvar, weights[i])) PY_SCIP_CALL(SCIPaddCons(self._scip, scip_cons)) pyCons = Constraint.create(scip_cons) @@ -6614,6 +6618,8 @@ cdef class Model: cdef SCIP_VAR* _binVar cdef SCIP_CONS* scip_cons cdef SCIP_Real coeff + cdef _VarArray wrapper + assert isinstance(cons, ExprCons), "given constraint is not ExprCons but %s" % cons.__class__.__name__ if cons._lhs is not None and cons._rhs is not None: @@ -6633,7 +6639,8 @@ cdef class Model: negate = True if binvar is not None: - _binVar = (binvar).scip_var + wrapper = _VarArray(binvar) + _binVar = wrapper.ptr[0] if not activeone: PY_SCIP_CALL(SCIPgetNegatedVar(self._scip, _binVar, &_binVar)) else: @@ -6644,10 +6651,9 @@ cdef class Model: terms = cons.expr.terms for key, coeff in terms.items(): - var = key[0] if negate: coeff = -coeff - PY_SCIP_CALL(SCIPaddVarIndicator(self._scip, scip_cons, var.scip_var, coeff)) + PY_SCIP_CALL(SCIPaddVarIndicator(self._scip, scip_cons, _VarArray(key[0]).ptr[0], coeff)) PY_SCIP_CALL(SCIPaddCons(self._scip, scip_cons)) pyCons = Constraint.create(scip_cons) @@ -8792,8 +8798,9 @@ cdef class Model: cdef SCIP_NODE* downchild cdef SCIP_NODE* eqchild cdef SCIP_NODE* upchild + cdef _VarArray wrapper = _VarArray(variable) - PY_SCIP_CALL(SCIPbranchVar(self._scip, (variable).scip_var, &downchild, &eqchild, &upchild)) + PY_SCIP_CALL(SCIPbranchVar(self._scip, wrapper.ptr[0], &downchild, &eqchild, &upchild)) return Node.create(downchild), Node.create(eqchild), Node.create(upchild) @@ -8821,8 +8828,9 @@ cdef class Model: cdef SCIP_NODE* downchild cdef SCIP_NODE* eqchild cdef SCIP_NODE* upchild + cdef _VarArray wrapper = _VarArray(variable) - PY_SCIP_CALL(SCIPbranchVarVal(self._scip, (variable).scip_var, value, &downchild, &eqchild, &upchild)) + PY_SCIP_CALL(SCIPbranchVarVal(self._scip, wrapper.ptr[0], value, &downchild, &eqchild, &upchild)) return Node.create(downchild), Node.create(eqchild), Node.create(upchild) @@ -9817,8 +9825,7 @@ cdef class Model: """ # no need to create a NULL solution wrapper in case we have a variable if sol == None and isinstance(expr, Variable): - var = expr - return SCIPgetSolVal(self._scip, NULL, var.scip_var) + return SCIPgetSolVal(self._scip, NULL, _VarArray(expr).ptr[0]) if sol == None: sol = Solution.create(self._scip, NULL) return sol[expr] @@ -10645,9 +10652,8 @@ cdef class Model: # avoid CONST term of Expr if term != CONST: assert len(term) == 1 - var = term[0] for i in range(nvars): - if vars[i] == var.scip_var: + if vars[i] == _VarArray(term[0]).ptr[0]: _coeffs[i] = coef PY_SCIP_CALL(SCIPchgReoptObjective(self._scip, objsense, vars, &_coeffs[0], nvars)) From 12a5b452a87903e7a2ad01f3fbb47a01e904cb5e Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Thu, 29 May 2025 00:51:57 +0200 Subject: [PATCH 14/21] fix double free --- src/pyscipopt/scip.pxi | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index f38c861be..6128423c1 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -6352,8 +6352,6 @@ cdef class Model: pyCons = Constraint.create(scip_cons) PY_SCIP_CALL(SCIPreleaseCons(self._scip, &scip_cons)) - free(_vars) - return pyCons def addConsOr(self, vars, resvar, name="", @@ -6480,8 +6478,6 @@ cdef class Model: pyCons = Constraint.create(scip_cons) PY_SCIP_CALL(SCIPreleaseCons(self._scip, &scip_cons)) - free(_vars) - return pyCons def addConsCardinality(self, consvars, cardval, indvars=None, weights=None, name="", From 6ce8dc185a42da5496b1e802e5f7d4c59bf410fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Dion=C3=ADsio?= <57299939+Joao-Dionisio@users.noreply.github.com> Date: Thu, 29 May 2025 17:39:27 +0100 Subject: [PATCH 15/21] Update src/pyscipopt/scip.pxi Co-authored-by: DominikKamp <130753997+DominikKamp@users.noreply.github.com> --- src/pyscipopt/scip.pxi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index 6128423c1..4ed0a9408 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -8275,7 +8275,7 @@ cdef class Model: cdef int i cdef _VarArray wrapper - assert sub_model.getNVars(False) >= self.getNVars(False), "The sub_model must have at least as many variables as the main model." + assert sub_model.getNVars(False) >= self.getNVars(True), "The sub_model must have at least as many variables as the main model." wrapper = _VarArray(sub_model.getVars()) vars = wrapper.ptr From 94032c465f2f842626e6fdfbad69ee4ce9d6b44e Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Thu, 29 May 2025 19:43:41 +0200 Subject: [PATCH 16/21] Fix leak --- src/pyscipopt/scip.pxi | 8 ++++---- tests/test_benders.py | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index 4ed0a9408..9014a0992 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -6332,7 +6332,7 @@ cdef class Model: """ cdef int nvars = len(vars) - cdef SCIP_VAR** _vars = malloc(nvars * sizeof(SCIP_VAR*)) + cdef SCIP_VAR** _vars cdef SCIP_VAR* _resvar cdef _VarArray resvar_wrapper = _VarArray(resvar) cdef _VarArray vars_wrapper = _VarArray(vars) @@ -6459,13 +6459,13 @@ cdef class Model: The newly created XOR constraint """ + assert type(rhsvar) is type(bool()), "Provide BOOLEAN value as rhsvar, you gave %s." % type(rhsvar) + cdef int nvars = len(vars) - cdef SCIP_VAR** _vars = malloc(nvars * sizeof(SCIP_VAR*)) + cdef SCIP_VAR** _vars cdef SCIP_CONS* scip_cons cdef int i cdef _VarArray wrapper = _VarArray(vars) - - assert type(rhsvar) is type(bool()), "Provide BOOLEAN value as rhsvar, you gave %s." % type(rhsvar) _vars = wrapper.ptr if name == '': diff --git a/tests/test_benders.py b/tests/test_benders.py index 2636b4c5e..fe8ded080 100644 --- a/tests/test_benders.py +++ b/tests/test_benders.py @@ -107,3 +107,5 @@ def test_flpbenders(): master.freeBendersSubproblems() assert 5.61e+03 - 10e-6 < master.getObjVal() < 5.61e+03 + 10e-6 + +test_flpbenders() \ No newline at end of file From bc443a49c0c6f00721ddfc8b62f64c2b3dd2323c Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Thu, 29 May 2025 19:56:28 +0200 Subject: [PATCH 17/21] typo --- tests/test_benders.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_benders.py b/tests/test_benders.py index fe8ded080..2636b4c5e 100644 --- a/tests/test_benders.py +++ b/tests/test_benders.py @@ -107,5 +107,3 @@ def test_flpbenders(): master.freeBendersSubproblems() assert 5.61e+03 - 10e-6 < master.getObjVal() < 5.61e+03 + 10e-6 - -test_flpbenders() \ No newline at end of file From bee82bb13bb551ddc8041d508097acd36561152e Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Thu, 29 May 2025 20:10:48 +0200 Subject: [PATCH 18/21] Clean tests a bit --- tests/test_bipartite.py | 2 -- tests/test_branch_probing_lp.py | 2 -- tests/test_model.py | 2 -- tests/test_numerics.py | 2 -- 4 files changed, 8 deletions(-) diff --git a/tests/test_bipartite.py b/tests/test_bipartite.py index a25253524..d7c2788ca 100644 --- a/tests/test_bipartite.py +++ b/tests/test_bipartite.py @@ -40,8 +40,6 @@ def branchexeclp(self, allowaddcons): - - def create_model(): scip = Model() scip.setHeuristics(SCIP_PARAMSETTING.OFF) diff --git a/tests/test_branch_probing_lp.py b/tests/test_branch_probing_lp.py index b8bde8aa9..fe5959190 100644 --- a/tests/test_branch_probing_lp.py +++ b/tests/test_branch_probing_lp.py @@ -39,7 +39,6 @@ def branchexeclp(self, allowaddcons): return {"result": SCIP_RESULT.BRANCHED} - def test_branching(): m = Model() m.setHeuristics(SCIP_PARAMSETTING.OFF) @@ -65,7 +64,6 @@ def test_branching(): m.addCons(quicksum(v for v in more_vars[50::2]) <= (40 - i) * quicksum(v for v in more_vars[405::2])) - m.addCons(r1 >= x0) m.addCons(r2 >= -x0) m.addCons(y0 == r1 +r2) diff --git a/tests/test_model.py b/tests/test_model.py index 85f7f069d..ee95e8e42 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -288,13 +288,11 @@ def test_getStage(): x = m.addVar() m.addCons(x >= 1) - print(m.getStage()) assert m.getStage() == SCIP_STAGE.PROBLEM assert m.getStageName() == "PROBLEM" m.optimize() - print(m.getStage()) assert m.getStage() == SCIP_STAGE.SOLVED assert m.getStageName() == "SOLVED" diff --git a/tests/test_numerics.py b/tests/test_numerics.py index 964e6c216..622f4c03a 100644 --- a/tests/test_numerics.py +++ b/tests/test_numerics.py @@ -18,5 +18,3 @@ def test_numerical_checks(): assert not m.isFeasGT(1, 0.99999) assert m.isGT(1, 0.99999) - -test_numerical_checks() From 5b940274348c38146ae45e27ea53eaa42a31bb14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Dion=C3=ADsio?= <57299939+Joao-Dionisio@users.noreply.github.com> Date: Thu, 5 Jun 2025 10:53:32 +0100 Subject: [PATCH 19/21] Apply suggestions from code review Co-authored-by: DominikKamp <130753997+DominikKamp@users.noreply.github.com> --- CHANGELOG.md | 2 +- src/pyscipopt/scip.pxi | 15 +++++---------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eda185ae9..05dbcd0aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ - Added isPositive(), isNegative(), isFeasLE(), isFeasLT(), isFeasGE(), isFeasGT(), isHugeValue(), and tests - Added SCIP_LOCKTYPE, addVarLocksType(), getNLocksDown(), getNLocksUp(), getNLocksDownType(), getNLocksUpType(), and tests ### Fixed -- Raised an error when logical constraints are used with expressions rather than variables. +- Raised an error when an expression is used when a variable is required ### Changed ### Removed diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index 9014a0992..cc0eb67ab 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -1042,8 +1042,6 @@ cdef class Solution: # fast track for Variable cdef SCIP_Real coeff - cdef _VarArray wrapper - if isinstance(expr, Variable): self._checkStage("SCIPgetSolVal") return SCIPgetSolVal(self.scip, self.sol, _VarArray(expr).ptr[0]) @@ -2410,13 +2408,11 @@ cdef class _VarArray: 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 = 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)) @@ -6466,6 +6462,7 @@ cdef class Model: cdef SCIP_CONS* scip_cons cdef int i cdef _VarArray wrapper = _VarArray(vars) + _vars = wrapper.ptr if name == '': @@ -8268,6 +8265,7 @@ cdef class Model: solution : Solution The corresponding solution in the main model """ + cdef SCIP_VAR** vars cdef SCIP_SOL* real_sol cdef SCIP_SOL* subscip_sol = sol.sol cdef SCIP_HEUR* _heur @@ -8277,12 +8275,9 @@ cdef class Model: assert sub_model.getNVars(False) >= self.getNVars(True), "The sub_model must have at least as many variables as the main model." - wrapper = _VarArray(sub_model.getVars()) - vars = wrapper.ptr - - name = str_conversion(heur.name) - _heur = SCIPfindHeur(self._scip, name) - PY_SCIP_CALL( SCIPtranslateSubSol(self._scip, sub_model._scip, subscip_sol, _heur, vars, &real_sol) ) + wrapper = _VarArray(sub_model.getVars(False)) + _heur = SCIPfindHeur(self._scip, str_conversion(heur.name)) + PY_SCIP_CALL( SCIPtranslateSubSol(self._scip, sub_model._scip, subscip_sol, _heur, wrapper.ptr, &real_sol) ) solution = Solution.create(self._scip, real_sol) return solution From c559c371f29997be6e8eabee9dfa247fb7050f8b Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Thu, 5 Jun 2025 15:01:32 +0100 Subject: [PATCH 20/21] Some more fixes --- src/pyscipopt/scip.pxi | 78 ++++++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 33 deletions(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index cc0eb67ab..afb7ab4b6 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -1042,16 +1042,20 @@ cdef class Solution: # fast track for Variable cdef SCIP_Real coeff + cdef _VarArray wrapper if isinstance(expr, Variable): + wrapper = _VarArray(expr) self._checkStage("SCIPgetSolVal") - return SCIPgetSolVal(self.scip, self.sol, _VarArray(expr).ptr[0]) + return SCIPgetSolVal(self.scip, self.sol, wrapper.ptr[0]) return sum(self._evaluate(term)*coeff for term, coeff in expr.terms.items() if coeff != 0) def _evaluate(self, term): self._checkStage("SCIPgetSolVal") result = 1 - for var in term.vartuple: - result *= SCIPgetSolVal(self.scip, self.sol, _VarArray(var).ptr[0]) + cdef _VarArray wrapper + wrapper = _VarArray(term.vartuple) + for i in range(len(term.vartuple)): + result *= SCIPgetSolVal(self.scip, self.sol, wrapper.ptr[i]) return result def __setitem__(self, Variable var, value): @@ -3445,6 +3449,7 @@ cdef class Model: cdef int nvars cdef SCIP_Real coef cdef int i + cdef _VarArray wrapper # turn the constant value into an Expr instance for further processing if not isinstance(expr, Expr): @@ -3469,7 +3474,8 @@ cdef class Model: # avoid CONST term of Expr if term != CONST: assert len(term) == 1 - PY_SCIP_CALL(SCIPchgVarObj(self._scip, _VarArray(term[0]).ptr[0], coef)) + wrapper = _VarArray(term[0]) + PY_SCIP_CALL(SCIPchgVarObj(self._scip, wrapper.ptr[0], coef)) if sense == "minimize": self.setMinimize() @@ -5206,6 +5212,7 @@ cdef class Model: cdef SCIP_CONS* scip_cons cdef SCIP_EXPR* prodexpr + cdef _VarArray wrapper PY_SCIP_CALL(SCIPcreateConsQuadraticNonlinear( self._scip, &scip_cons, str_conversion(kwargs['name']), 0, NULL, NULL, # linear @@ -5217,13 +5224,16 @@ cdef class Model: for v, c in terms.items(): if len(v) == 1: # linear - PY_SCIP_CALL(SCIPaddLinearVarNonlinear(self._scip, scip_cons, _VarArray(v[0]).ptr[0], c)) + wrapper = _VarArray(v[0]) + PY_SCIP_CALL(SCIPaddLinearVarNonlinear(self._scip, scip_cons, wrapper.ptr[0], c)) else: # nonlinear assert len(v) == 2, 'term length must be 1 or 2 but it is %s' % len(v) varexprs = malloc(2 * sizeof(SCIP_EXPR*)) - PY_SCIP_CALL( SCIPcreateExprVar(self._scip, &varexprs[0], _VarArray(v[0]).ptr[0], NULL, NULL) ) - PY_SCIP_CALL( SCIPcreateExprVar(self._scip, &varexprs[1], _VarArray(v[1]).ptr[0], NULL, NULL) ) + wrapper = _VarArray(v[0]) + PY_SCIP_CALL( SCIPcreateExprVar(self._scip, &varexprs[0], wrapper.ptr[0], NULL, NULL) ) + wrapper = _VarArray(v[1]) + PY_SCIP_CALL( SCIPcreateExprVar(self._scip, &varexprs[1], wrapper.ptr[0], NULL, NULL) ) PY_SCIP_CALL( SCIPcreateExprProduct(self._scip, &prodexpr, 2, varexprs, 1.0, NULL, NULL) ) PY_SCIP_CALL( SCIPaddExprNonlinear(self._scip, scip_cons, prodexpr, c) ) @@ -5351,9 +5361,7 @@ cdef class Model: for node in nodes: if node[0] == Operator.varidx: nvars += 1 - vars = malloc(nvars * sizeof(SCIP_VAR*)) - varpos = 0 scipexprs = malloc(len(nodes) * sizeof(SCIP_EXPR*)) for i,node in enumerate(nodes): opidx = node[0] @@ -5361,9 +5369,7 @@ cdef class Model: assert len(node[1]) == 1 pyvar = node[1][0] # for vars we store the actual var! wrapper = _VarArray(pyvar) - vars[varpos] = wrapper.ptr[0] - PY_SCIP_CALL( SCIPcreateExprVar(self._scip, &scipexprs[i], vars[varpos], NULL, NULL) ) - varpos += 1 + PY_SCIP_CALL( SCIPcreateExprVar(self._scip, &scipexprs[i], wrapper.ptr[0], NULL, NULL) ) continue if opidx == Operator.const: assert len(node[1]) == 1 @@ -5422,7 +5428,6 @@ cdef class Model: continue # default: raise NotImplementedError - assert varpos == nvars # create nonlinear constraint for the expression root PY_SCIP_CALL( SCIPcreateConsNonlinear( @@ -5447,7 +5452,6 @@ cdef class Model: # free more memory free(scipexprs) - free(vars) return PyCons @@ -6140,13 +6144,12 @@ cdef class Model: if name == '': name = 'c'+str(SCIPgetNConss(self._scip)+1) + wrapper = _VarArray(vars) for i in range(nvars): - wrapper = _VarArray(vars[i]) - vars_array[i] = wrapper.ptr[0] weights_array[i] = weights[i] PY_SCIP_CALL(SCIPcreateConsKnapsack( - self._scip, &scip_cons, str_conversion(name), nvars, vars_array, weights_array, + self._scip, &scip_cons, str_conversion(name), nvars, wrapper.ptr, weights_array, capacity, initial, separate, enforce, check, propagate, local, modifiable, dynamic, removable, stickingatnode)) @@ -6204,6 +6207,7 @@ cdef class Model: cdef SCIP_CONS* scip_cons cdef int nvars cdef int i + cdef _VarArray wrapper PY_SCIP_CALL(SCIPcreateConsSOS1(self._scip, &scip_cons, str_conversion(name), 0, NULL, NULL, initial, separate, enforce, check, propagate, local, dynamic, removable, stickingatnode)) @@ -6211,13 +6215,14 @@ cdef class Model: if name == '': name = 'c'+str(SCIPgetNConss(self._scip)+1) + nvars = len(vars) + wrapper = _VarArray(vars) if weights is None: - for v in vars: - PY_SCIP_CALL(SCIPappendVarSOS1(self._scip, scip_cons, _VarArray(v).ptr[0])) + for i in range(nvars): + PY_SCIP_CALL(SCIPappendVarSOS1(self._scip, scip_cons, wrapper.ptr[i])) else: - nvars = len(vars) for i in range(nvars): - PY_SCIP_CALL(SCIPaddVarSOS1(self._scip, scip_cons, _VarArray(vars[i]).ptr[0], weights[i])) + PY_SCIP_CALL(SCIPaddVarSOS1(self._scip, scip_cons, wrapper.ptr[i], weights[i])) PY_SCIP_CALL(SCIPaddCons(self._scip, scip_cons)) @@ -6267,6 +6272,7 @@ cdef class Model: cdef SCIP_CONS* scip_cons cdef int nvars cdef int i + cdef _VarArray wrapper PY_SCIP_CALL(SCIPcreateConsSOS2(self._scip, &scip_cons, str_conversion(name), 0, NULL, NULL, initial, separate, enforce, check, propagate, local, dynamic, removable, stickingatnode)) @@ -6274,13 +6280,14 @@ cdef class Model: if name == '': name = 'c'+str(SCIPgetNConss(self._scip)+1) + nvars = len(vars) + wrapper = _VarArray(vars) if weights is None: - for v in vars: - PY_SCIP_CALL(SCIPappendVarSOS2(self._scip, scip_cons, _VarArray(v).ptr[0])) + for i in range(nvars): + PY_SCIP_CALL(SCIPappendVarSOS2(self._scip, scip_cons, wrapper.ptr[i])) else: - nvars = len(vars) for i in range(nvars): - PY_SCIP_CALL(SCIPaddVarSOS2(self._scip, scip_cons, _VarArray(vars[i]).ptr[0], weights[i])) + PY_SCIP_CALL(SCIPaddVarSOS2(self._scip, scip_cons, wrapper.ptr[i], weights[i])) PY_SCIP_CALL(SCIPaddCons(self._scip, scip_cons)) @@ -6394,13 +6401,13 @@ cdef class Model: cdef int nvars = len(vars) cdef SCIP_VAR** _vars cdef SCIP_VAR* _resvar + cdef _VarArray resvar_wrapper = _VarArray(resvar) + cdef _VarArray vars_wrapper = _VarArray(vars) cdef SCIP_CONS* scip_cons cdef int i - cdef _VarArray wrapper = _VarArray(vars) - cdef _VarArray resvar_wrapper = _VarArray(resvar) _resvar = resvar_wrapper.ptr[0] - _vars = wrapper.ptr + _vars = vars_wrapper.ptr if name == '': name = 'c'+str(SCIPgetNConss(self._scip)+1) @@ -6542,9 +6549,9 @@ cdef class Model: if name == '': name = 'c'+str(SCIPgetNConss(self._scip)+1) + v_wrapper = _VarArray(consvars) for i, v in enumerate(consvars): - v_wrapper = _VarArray(v) - scip_var = v_wrapper.ptr[0] + scip_var = v_wrapper.ptr[i] if indvars: indvar_wrapper = _VarArray(indvars[i]) indvar = indvar_wrapper.ptr[0] @@ -6646,7 +6653,8 @@ cdef class Model: for key, coeff in terms.items(): if negate: coeff = -coeff - PY_SCIP_CALL(SCIPaddVarIndicator(self._scip, scip_cons, _VarArray(key[0]).ptr[0], coeff)) + wrapper = _VarArray(key[0]) + PY_SCIP_CALL(SCIPaddVarIndicator(self._scip, scip_cons, wrapper.ptr[0], coeff)) PY_SCIP_CALL(SCIPaddCons(self._scip, scip_cons)) pyCons = Constraint.create(scip_cons) @@ -9815,8 +9823,10 @@ cdef class Model: """ # no need to create a NULL solution wrapper in case we have a variable + cdef _VarArray wrapper if sol == None and isinstance(expr, Variable): - return SCIPgetSolVal(self._scip, NULL, _VarArray(expr).ptr[0]) + wrapper = _VarArray(expr) + return SCIPgetSolVal(self._scip, NULL, wrapper.ptr[0]) if sol == None: sol = Solution.create(self._scip, NULL) return sol[expr] @@ -10617,6 +10627,7 @@ cdef class Model: cdef SCIP_OBJSENSE objsense cdef SCIP_Real coef cdef int i + cdef _VarArray wrapper if sense == "minimize": objsense = SCIP_OBJSENSE_MINIMIZE @@ -10644,7 +10655,8 @@ cdef class Model: if term != CONST: assert len(term) == 1 for i in range(nvars): - if vars[i] == _VarArray(term[0]).ptr[0]: + wrapper = _VarArray(term[0]) + if vars[i] == wrapper.ptr[0]: _coeffs[i] = coef PY_SCIP_CALL(SCIPchgReoptObjective(self._scip, objsense, vars, &_coeffs[0], nvars)) From f6206f61d5fb942d6579d8fc3ac4fca7ab680b6b Mon Sep 17 00:00:00 2001 From: Joao-Dionisio Date: Thu, 5 Jun 2025 15:20:52 +0100 Subject: [PATCH 21/21] Remaining comments --- src/pyscipopt/scip.pxi | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index afb7ab4b6..3b2ee7db7 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -5274,22 +5274,16 @@ cdef class Model: terms = cons.expr.terms # collect variables - variables = {var.ptr(): var for term in terms for var in term} - variables = list(variables.values()) - varindex = {var.ptr(): i for (i, var) in enumerate(variables)} + variables = {i: [var for var in term] for i, term in enumerate(terms)} # create monomials for terms monomials = malloc(len(terms) * sizeof(SCIP_EXPR*)) termcoefs = malloc(len(terms) * sizeof(SCIP_Real)) for i, (term, coef) in enumerate(terms.items()): - termvars = malloc(len(term) * sizeof(SCIP_VAR*)) - for j, var in enumerate(term): - wrapper = _VarArray(var) - termvars[j] = wrapper.ptr[0] + wrapper = _VarArray(variables[i]) - PY_SCIP_CALL( SCIPcreateExprMonomial(self._scip, &monomials[i], len(term), termvars, NULL, NULL, NULL) ) + PY_SCIP_CALL( SCIPcreateExprMonomial(self._scip, &monomials[i], wrapper.size, wrapper.ptr, NULL, NULL, NULL) ) termcoefs[i] = coef - free(termvars) # create polynomial from monomials PY_SCIP_CALL( SCIPcreateExprSum(self._scip, &expr, len(terms), monomials, termcoefs, 0.0, NULL, NULL)) @@ -6134,7 +6128,6 @@ cdef class Model: cdef int nvars = len(vars) cdef int i - cdef SCIP_VAR** vars_array = malloc(nvars * sizeof(SCIP_VAR*)) cdef SCIP_Longint* weights_array = malloc(nvars * sizeof(SCIP_Real)) cdef SCIP_CONS* scip_cons cdef _VarArray wrapper @@ -6153,7 +6146,6 @@ cdef class Model: capacity, initial, separate, enforce, check, propagate, local, modifiable, dynamic, removable, stickingatnode)) - free(vars_array) free(weights_array) PY_SCIP_CALL(SCIPaddCons(self._scip, scip_cons))