fix: ignore masked variable labels when matching constraints - #886
fix: ignore masked variable labels when matching constraints#886YassineAbdelouadoud wants to merge 2 commits into
Conversation
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | test_to_lp[storage-n=250] |
28.2 MB | 36.3 MB | -22.27% |
| ❌ | Memory | test_to_lp[merge_balance-severity=0] |
2.6 MB | 3.2 MB | -18.94% |
| ⚡ | Memory | test_to_lp[storage-n=10] |
2.6 MB | 2 MB | +30.79% |
| ⚡ | Memory | test_to_lp[rolling-severity=50] |
429.7 MB | 344 MB | +24.93% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing YassineAbdelouadoud:fix-remove-variables-masked (58fca82) with master (5d22e35)
Footnotes
-
175 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
`Model.remove_variables` deleted every constraint whose `has_variable` matched the removed variable. For a masked variable that was every constraint with a padded term slot: `-1` marks a masked entry on the variable side and an empty term slot on the constraint side, and the two were compared without filtering the sentinel. Filter the -1 entries out of the variable labels before matching, via a new `common.assigned_labels` helper, in both `has_variable` implementations and in the objective cleanup of `remove_variables`. Only the dense `Constraint` path was actually affected -- a CSR matrix stores no empty slots -- but the same guard now holds for both. Fixes PyPSA#883 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7d2dc5e to
f884458
Compare
|
thanks @YassineAbdelouadoud I found another bug that I fixed in #895 with these changes in one go ; closing this. thanks for raising and proposing the fix. I will make a bug fix release |
I encountered this issue while updating to the latest linopy version and running the POMMES framework
Fixes #883
Note
The following content was generated by AI.
The bug
Model.remove_variablesremoves every constraint for whichconstraint.has_variable(variable)is true.has_variablecompared theconstraint's
varsagainst the variable's labels without filtering the-1sentinel, which means two different things on the two sides:
-1marks a masked entry;vars,-1marks an empty (padded) term slot.So any masked variable matched any constraint that has a padded term slot, even
when the two are entirely unrelated, and the constraint was silently deleted.
Models built with
mask=can therefore lose constraints and solve to a wrongoptimum or unbounded.
The fix
Filter the
-1entries out of the variable labels before matching, through anew
common.assigned_labelshelper, applied at the three places that match avariable's labels against another object's labels:
Constraint.has_variable(dense path) — the one that was actually broken;CSRConstraint.has_variable— a CSR matrix stores no empty slots, so thispath was already correct; the guard is added for symmetry;
remove_variables, which had the samelatent mismatch (there it only dropped already-empty terms).
Test
test_remove_masked_variable_keeps_unrelated_constraintsintest/test_model.py,parametrized over
freezeto cover both constraint representations. It builds aconstraint over a masked variable
b(hence with padded term slots) plus anunmasked
c, and a second constraint that genuinely uses the maskeda, thenremoves
a: the first constraint must survive, the second must still go.On master the
freeze=Falsecase fails atassert not without_a.has_variable(a);the
freeze=Truecase passes before and after.Verification
test/remotewas skipped locally because theoetcextra(
google-cloud-storage) is not installed in this environment; it is unrelatedto this change.
Reproduction on master, before the fix: