Fix duplicate-variable coefficients in LinExpr - #429
Merged
Merged
Conversation
…uctor Issue #396 / PR #397: constructing a LinExpr directly with a variables list containing the same Var more than once (e.g. via Var.__add__, Var.__sub__, Var.__eq__/__le__/__ge__ on identical objects, or manual LinExpr(variables=..., coeffs=...) calls) silently kept only the last coefficient because dict(zip(...)) overwrites duplicate keys. PR #397's approach patches this only for the specific case where a Var is combined with itself in a handful of dunder methods, and explicitly does not fix direct constructor calls with 3+ duplicate entries (see its commented-out test case). This fixes the root cause in the LinExpr constructor itself, covering all call sites. To avoid regressing performance for the common duplicate-free case, a cheap len(variables) == len(set(variables)) check picks the fast dict(zip(...)) path when there are no duplicates, falling back to an accumulating loop only when needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
What this fixes
Thank you to the contributor of #397 for finding and documenting this problem.
The current LinExpr constructor uses dict(zip(variables, coeffs)). If the same variable appears more than once, earlier coefficients are silently lost. This can affect direct LinExpr construction as well as expressions involving identical variables.
This PR fixes the problem in the constructor itself by adding duplicate coefficients together. It keeps the fast path for the usual case where variables are unique.
It also adds tests for duplicate variables, including direct construction with three repeated entries.
Tests: 68 passed, 1 skipped.
Could you please check whether these cases fix the problem you reported in #397?