Add support for knapsack constraints

[Joao-Dionisio]

Fix #941

Still not done, but this is pretty fun, in a turn off your brain and code kind of way.

Currently having some problems with the compilation, but likely due to my local SCIP, will re-check later.

[Copilot]

Pull Request Overview

This PR adds support for knapsack constraints by allowing an optional flag in addCons, and updates tests to validate the new functionality.

• Introduces a new test function for knapsack constraints in tests/test_cons.py
• Modifies tests for linear constraint coefficient retrieval in tests/test_cons.py
• Updates CHANGELOG.md to document the new knapsack constraint support

Reviewed Changes

Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.

File	Description
tests/test_cons.py	Added tests for knapsack constraint flag and related methods
CHANGELOG.md	Updated changelog to include knapsack constraint support

Files not reviewed (2)

• src/pyscipopt/scip.pxd: Language not supported
• src/pyscipopt/scip.pxi: Language not supported

Comments suppressed due to low confidence (2)

tests/test_cons.py:225

• Consider adding negative test cases for constraints without the knapsack flag to verify that knapsack-specific methods are not available in non-knapsack constraints.

knapsack_cons = m.addCons(4*x + 2*y <= 10, knapsack=True)

tests/test_cons.py:251

• Clarify and assert the expected coefficient order from getValsLinear to ensure consistency, especially if constraint transformations occur during optimization.

assert m.getValsLinear(c1) == [2,1]

[Joao-Dionisio]

Updated the default values of the logical constraint names, because they would cause issues (like the one that happened with the indicator constraints - can't find the issue atm)

[DominikKamp]

Values are SCIP_Longint, not SCIP_Real, and should accept Python int.

[DominikKamp]

Can someone tell copilot that pxi and pxd is Python?

[Joao-Dionisio]

Thank you, @DominikKamp! By the way, how do you feel about the default names for indicator and logical constraints? Ie, if you add 1 AND constraint and two OR constraints without naming them, they default to "c1AND", "c2OR", "c3OR".

Also, should we try to lump all the methods together? For example, should getRhs work with every handler, or should we really just have a getRhsLinear (but without changing the name), getKnapsackCapacity, etc.?

And finally, yeah Copilot is still a bit dummy, unfortunately. Maybe next year :)

[DominikKamp]

If there is a common counter for constraints, it should not be necessary to add the constraint type to the name if there is a way to get the name of the constraint handler separately (without it, this is also more like the generic names in SCIP).

Yes, there should be wrappers for SCIPconsGet...() but some special constraints require additional methods like for knapsack to get the integral values.

[Joao-Dionisio]

Okay, everything seems to be working now. I decided to keep the same structure as the other constraint handlers, and one needs to do m.addConsKnapsack([var1, var2], [weight1, weight2], capacity), instead of m.addConsKnapsack(weight1*var1 + weight2*var2 <= capacity). I don't mind it too much, it somewhat distinguishes from the "default" linear handler.

@DominikKamp, there doesn't seem to be a check for the variables being binary on the SCIP side, despite the constraint handler's description. Is this intentional or an oversight?

EDIT: My bad, it is there, I was just not running SCIP in debug mode. Still, feels strange to be able to use knapsack constraints with continuous variables and then getting incorrect results. Maybe the check should be in pyscipopt?

[Copilot]

Pull Request Overview

This pull request introduces support for knapsack constraints by adding new knapsack-specific methods in both the Python layer and its C interface, and extends the test suite to verify correct behavior.

• New functions for creating, modifying, and querying knapsack constraints have been added (e.g. addConsKnapsack, isKnapsack, addCoefKnapsack, getWeightsKnapsack, getDualsolKnapsack, and getDualfarkasKnapsack).
• Test cases for the knapsack constraint functionality have been implemented in tests/test_cons.py.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
tests/test_cons.py	New tests verifying knapsack creation, coefficient addition, and dual values.
src/pyscipopt/scip.pxi	New knapsack constraint methods implemented and integrated into Model and Constraint interfaces.
src/pyscipopt/scip.pxd	External declarations for knapsack functions have been added.
CHANGELOG.md	Changelog updated to mention knapsack constraint support.

[Joao-Dionisio]

@mmghannam can you take a quick look? Mostly on the design decisions, which I'd pass on to the other handlers as well.

m.addConsKnapsack([var1, var2], [weight1, weight2], capacity), instead of m.addConsKnapsack(weight1*var1 + weight2*var2 <= capacity). And also, besides adding things like getCapacityKnapsack, also have the option to use it from getRhs, which is now checking for the specific constraint handler.

[DominikKamp]

there doesn't seem to be a check for the variables being binary on the SCIP side, despite the constraint handler's description. Is this intentional or an oversight?

Currently, the knapsack constraint handler expects binary variables and there are multiple assertions on this. But there should also be a direct error because the knapsacks I have seen so far can usually handle multiple copies of an item so it can hardly be inferred that integer variables are not supported. This should happen in SCIP as already for AND, OR, XOR, LogicOR, and SetPPC constraints.

[Joao-Dionisio]

@DominikKamp I was more concerned with it not being a direct error with continuous variables. This is with SCIP 9.2.3, so it might not apply in master.

test_cons_knapsack_cip.zip

feasible solution found by trivial heuristic after 0.0 seconds, objective value 0.000000e+00
presolving:
(round 1, fast)       2 del vars, 1 del conss, 0 add conss, 0 chg bounds, 0 chg sides, 0 chg coeffs, 0 upgd conss, 0 impls, 0 clqs
presolving (2 rounds: 2 fast, 1 medium, 1 exhaustive):
 3 deleted vars, 1 deleted constraints, 0 added constraints, 0 tightened bounds, 0 added holes, 0 changed sides, 0 changed coefficients
 0 implications, 0 cliques
transformed 1/2 original solutions to the transformed problem space
Presolving Time: 0.00

SCIP Status        : problem is solved [optimal solution found]
Solving Time (sec) : 0.00
Solving Nodes      : 0
Primal Bound       : -2.00000000000000e+00 (2 solutions)
Dual Bound         : -2.00000000000000e+00
Gap                : 0.00 %
  [knapsack] <c1>: +4<x>[C] +2<y>[C] +3<z>[C] <= 5;
violation: the capacity is violated by 3
best solution is not feasible in original problem

[DominikKamp]

Any non-binary variable can lead to wrong results, will add an error later.

[mmghannam]

@mmghannam can you take a quick look? Mostly on the design decisions, which I'd pass on to the other handlers as well.

m.addConsKnapsack([var1, var2], [weight1, weight2], capacity), instead of m.addConsKnapsack(weight1*var1 + weight2*var2 <= capacity). And also, besides adding things like getCapacityKnapsack, also have the option to use it from getRhs, which is now checking for the specific constraint handler.

• I think passing a list of tuples of (var, weight) is better; it avoids the error of passing different sized lists.
• For your second point, I agree if getRhs is defined for the knapsack constraints, but if you mean you allow that to be called only in pyscipopt and it would call getCapactyKnapsack inside, I think that would could be confusing, maybe not for this constraint handler but maybe others.

[Joao-Dionisio]

I think passing a list of tuples of (var, weight) is better; it avoids the error of passing different sized lists.

Hmm I don't know, if we're going to change SCIP's arguments, then imo it should be for something that's easier to use. To me, the tuple way seems more cumbersome.
If the length mismatch is the concern, then what about adding an assert?

Agreed on the getRhs.

[mmghannam]

I think passing a list of tuples of (var, weight) is better; it avoids the error of passing different sized lists.

Hmm I don't know, if we're going to change SCIP's arguments, then imo it should be for something that's easier to use. To me, the tuple way seems more cumbersome. If the length mismatch is the concern, then what about adding an assert?

Yes adding an assert is better :)

[Joao-Dionisio]

How do you feel about adding the missing methods? They are SCIPgetRowKnapsack, SCIPsolveKnapsackExactly, SCIPsolveKnapsackApproximately, SCIPseparateKnapsackCuts, SCIPseparateRelaxedKnapsack, SCIPcleanupConssKnapsack.

On one hand, having more functionality is nice. But maybe these don't belong in the Python interface, and would clutter it maybe unnecessarily. A question of whether PySCIPOpt should start catering to more advanced users, or keep making it somewhat beginner/intermediate friendly.

EDIT: okay, going to agree with the past-me. "it's not reasonable to expect all methods to be interfaced, let alone tested." In a future PR, this may be discussed again, but it makes more sense to have basic functionality first.

[DominikKamp]

Why was the getRhs() method not generalized to knapsack finally?

[Joao-Dionisio]

@DominikKamp Mo's opinion:

I agree if getRhs is defined for the knapsack constraints, but if you mean you allow that to be called only in pyscipopt and it would call getCapactyKnapsack inside, I think that would could be confusing, maybe not for this constraint handler but maybe others.

Which I agree with. I think in general, if the naming is not the same, for consistency I'll keep them separate. If it was called SCIPgetRhsKnapsack, I would generalize. How do you feel about this reasoning? (I know I'm not following this, even in this PR)

[DominikKamp]

I just understood Mo differently, to let getRhs() call SCIPconsGetRhs() and do not care about the specific linear type here, also wrappers for SCIPgetConsVars() and SCIPgetConsVals() seem to be missing.

[Joao-Dionisio]

Ah, yeah I can do that for getRhs().

Do you mean wrapping SCIPgetVarsKnapsack? I figured that since we already have getConsVars, then it wouldn't make sense to repeat. Agreed with SCIPgetConsVals being missing, I'll add it now.

EDIT: My god, there are so many repeat methods, I'm just now realizing.

From the documentation SCIPgetConsVals

returns the value array of an arbitrary SCIP constraint that can be represented as a single linear constraint

With such a generic name, I think people would incorrectly assume that any constraint is supported. I don't know if wrapping this one makes sense.

EDIT2: I should have looked better before commenting. So you're in favor of just splitting between nonlinear constraints and all others?

Something like

if constype == "nonlinear":
    _vars = SCIPgetRhsNonlinear(...)
else:
   _vars = SCIPconsGetRhs(...)

?

[DominikKamp]

Indeed, getConsVars() is already there. So it should be possible to provide getConsVals() in a similar way. For the sides your suggestion makes sense.