From 874c6b1118d1ffb852d99d5ea45c6f9d7d1d17b2 Mon Sep 17 00:00:00 2001 From: Mohit Prajapati Date: Tue, 15 Sep 2026 16:32:32 +0530 Subject: [PATCH] Fix dual accessors for original knapsack constraints getDualsolKnapsack and getDualfarkasKnapsack looked up the transformed constraint the wrong way round, so an original constraint was passed to SCIP directly and the dual came back as 0. Follow the linear versions instead, and return 0 when presolve has removed the constraint and there is no transformed one to read from. getVarsAnd asserted a two-element tuple, which never fails. Drop the parentheses so a non-AND constraint is rejected. --- CHANGELOG.md | 1 + src/pyscipopt/scip.pxi | 24 ++++++++++++++------ tests/test_cons.py | 51 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44af6c4b0..9307dfacb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - Added type annotations to most methods on the `Model` class - Added tests for `getRowLinear()` and extended existing testing for `isActive()` ### Fixed +- Fixed `getDualsolKnapsack()` and `getDualfarkasKnapsack()` returning 0 for original constraints, and the handler check in `getVarsAnd()` that could never fail - Fixed Cython 3.3 compatibility (#1248) - Made `test_markDoNotAggrVar_and_getStatus` robust to SCIP presolve changes by discovering the aggregated/multi-aggregated variables instead of hardcoding them ### Changed diff --git a/src/pyscipopt/scip.pxi b/src/pyscipopt/scip.pxi index cc2722af9..476ca4ca9 100644 --- a/src/pyscipopt/scip.pxi +++ b/src/pyscipopt/scip.pxi @@ -6953,7 +6953,7 @@ cdef class Model: cdef int i constype = bytes(SCIPconshdlrGetName(SCIPconsGetHdlr(and_cons.scip_cons))).decode('UTF-8') - assert(constype == 'and', "The constraint handler %s does not have this functionality." % constype) + assert constype == 'and', "The constraint handler %s does not have this functionality." % constype nvars = SCIPgetNVarsAnd(self._scip, and_cons.scip_cons) _vars = SCIPgetVarsAnd(self._scip, and_cons.scip_cons) @@ -8960,14 +8960,19 @@ cdef class Model: float """ + cdef SCIP_CONS* transcons + constype = bytes(SCIPconshdlrGetName(SCIPconsGetHdlr(cons.scip_cons))).decode('UTF-8') if not constype == 'knapsack': raise Warning("dual solution values not available for constraints of type ", constype) if cons.isOriginal(): - transcons = cons + # presolve may have removed it, and then there's no dual to read + PY_SCIP_CALL(SCIPgetTransformedCons(self._scip, cons.scip_cons, &transcons)) + if transcons == NULL: + return 0.0 else: - transcons = self.getTransformedCons(cons) - return SCIPgetDualsolKnapsack(self._scip, transcons.scip_cons) + transcons = cons.scip_cons + return SCIPgetDualsolKnapsack(self._scip, transcons) def getDualMultiplier(self, Constraint cons): """ @@ -9020,12 +9025,17 @@ cdef class Model: float """ + cdef SCIP_CONS* transcons + # TODO this should ideally be handled on the SCIP side if cons.isOriginal(): - return SCIPgetDualfarkasKnapsack(self._scip, cons.scip_cons) + # presolve may have removed it, and then there's no Farkas value to read + PY_SCIP_CALL(SCIPgetTransformedCons(self._scip, cons.scip_cons, &transcons)) + if transcons == NULL: + return 0.0 + return SCIPgetDualfarkasKnapsack(self._scip, transcons) else: - transcons = self.getTransformedCons(cons) - return SCIPgetDualfarkasKnapsack(self._scip, transcons.scip_cons) + return SCIPgetDualfarkasKnapsack(self._scip, cons.scip_cons) def getVarRedcost(self, Variable var): """ diff --git a/tests/test_cons.py b/tests/test_cons.py index 8f577118c..294be5f5b 100644 --- a/tests/test_cons.py +++ b/tests/test_cons.py @@ -352,6 +352,57 @@ def test_cons_knapsack(): assert m.getDualsolKnapsack(knapsack_cons) == 0 assert m.getDualfarkasKnapsack(knapsack_cons) == 0 + +def _lp_only_model(): + # presolve, heuristics, separation and propagation off, so the constraint + # stays in the LP and its dual values are still there after the solve + m = Model() + m.hideOutput() + m.setPresolve(SCIP_PARAMSETTING.OFF) + m.setHeuristics(SCIP_PARAMSETTING.OFF) + m.setSeparating(SCIP_PARAMSETTING.OFF) + m.disablePropagation() + return m + + +def test_cons_knapsack_dualsol_on_original_constraint(): + m = _lp_only_model() + x = m.addVar(vtype="B") + y = m.addVar(vtype="B") + knapsack_cons = m.addConsKnapsack([x, y], [1, 1], 1) + m.setObjective(-x - y, "minimize") + m.optimize() + + # the knapsack is binding and its dual is unique here, so asking through + # the original constraint has to give the same value as the transformed one + transformed = m.getTransformedCons(knapsack_cons) + assert m.isEQ(m.getDualsolKnapsack(knapsack_cons), -1.0) + assert m.isEQ(m.getDualsolKnapsack(knapsack_cons), m.getDualsolKnapsack(transformed)) + + +def test_cons_knapsack_dualfarkas_on_original_constraint(): + m = _lp_only_model() + x = m.addVar(vtype="B") + y = m.addVar(vtype="B") + knapsack_cons = m.addConsKnapsack([x, y], [1, 1], 1) + m.addCons(x + y >= 2) + m.optimize() + + assert m.getStatus() == "infeasible" + transformed = m.getTransformedCons(knapsack_cons) + assert not m.isZero(m.getDualfarkasKnapsack(knapsack_cons)) + assert m.isEQ(m.getDualfarkasKnapsack(knapsack_cons), m.getDualfarkasKnapsack(transformed)) + + +def test_getVarsAnd_rejects_other_constraint_types(): + m = Model() + x = m.addVar(vtype="B") + linear_cons = m.addCons(x <= 1) + + with pytest.raises(AssertionError): + m.getVarsAnd(linear_cons) + + def test_cons_cumulative(): """Three jobs on a resource with capacity 3 must not overlap in demand.