Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the review on #1255.
getDualsolKnapsack()andgetDualfarkasKnapsack()had the original/transformed lookup the wrong way round, so passing the constraint you created handed the original one straight to SCIP and you got 0 back. On a model where the knapsack is binding (x + y <= 1, minimising -x - y, presolve off), the dual through the original constraint came back as 0.0, while the transformed one and the same model written as a linear constraint both give -1.0. The Farkas value on an infeasible version did the same. Both now look up the transformed constraint, following the linear versions.getVarsAnd()usedassert(cond, msg), which asserts a two element tuple and never fails, so a non-AND constraint went straight through toSCIPgetNVarsAndand SCIP just printed "constraint is not an AND-constraint". I removed the parentheses.The three new tests fail on master and pass here.
One thing came up that I'd like your view on. Fixing the lookup broke the existing
test_cons_knapsack: presolve removes the constraint there, so there's no transformed constraint andgetTransformedCons()raises "cannot create Constraint with SCIP_CONS* == NULL". The old code only passed that test because it never asked for the transformed one. I left the test as it is and made the knapsack functions return 0.0 when the transformed constraint is gone, which is also what SCIP gives for a constraint with no LP row.getDualsolLinear()andgetDualfarkasLinear()already raise in that situation, and the logicor ones in #1255 will too, since they use the same lookup. Would you rather they all return 0 there, or all raise? I can make them consistent either way, here or in #1255.Tested locally with SCIP 10.0.3: 452 passed, and stubtest is clean.