fix: support adding/removing variables after adding frozen constraint#806
Open
koen-vg wants to merge 2 commits into
Open
fix: support adding/removing variables after adding frozen constraint#806koen-vg wants to merge 2 commits into
koen-vg wants to merge 2 commits into
Conversation
Merging this PR will not alter performance
Comparing 
Footnotes
|
Frozen (CSR-backed) constraints cache their matrix at freeze time, sized against the active-variable set as it was then. Adding variables afterwards (e.g. an auxiliary variable introduced by a constraint added after this one was frozen, common under Model(freeze_constraints=True) when building incrementally) grows the active-variable count, but CSRConstraint.to_matrix and to_matrix_with_rhs returned the cached CSR verbatim, ignoring the VariableLabelIndex passed in. The earlier-frozen blocks stay narrower than later ones, so stacking them fails with "ValueError: incompatible dimensions for axis 1". This breaks the invariant documented in PyPSA#630 -- to_matrix on both the mutable and frozen constraint "always returns a csr matrix of shape (n_active_cons, n_active_vars)" -- which the VariableLabelIndex exists precisely to uphold. Widen the cached CSR to the current n_active_vars in a shared _csr_for_index helper used by both to_matrix (matrix-accessor path) and to_matrix_with_rhs (direct-solve path). Variable positions are assigned in encounter order and are stable under additions, so widening only appends empty trailing columns. Adds differential tests (freeze vs mutable must agree) covering the baseline, variables added after freeze, and constraints linking later-added variables.
Frozen (CSR-backed) constraints store absolute dense variable positions from freeze time. Removing a variable renumbers the positions of every later variable, but Model.remove_variables left surviving frozen constraints with those stale positions -- both the wrong width and the wrong columns -- producing a corrupt constraint matrix (e.g. shape (2, 5) instead of (2, 2)) or an IndexError at solve time. The mutable Constraint path handles removal correctly, so this was a frozen-vs-mutable parity gap. remove_variables now captures the pre-removal variable ordering and, once the variable (and any constraints referencing it) are gone, remaps each surviving frozen constraint's CSR columns through variable labels via the new CSRConstraint._remap_columns. The old ordering is still available at the mutation site, so no per-constraint label storage is needed; a frozen constraint that referenced the removed variable has already been dropped, so every remaining column maps to a valid new position. Extends the differential tests with removal scenarios: first/middle-variable removal, straddling references, sequential removals, remove-then-add, and the drop-referencing-constraint parity case.
koen-vg
force-pushed
the
fix/frozen-csr-active-vars
branch
from
b690853 to
b2b622c
Compare
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.
I like the new
CSRConstraintand the memory benefits it comes with. The current implementation, however, breaks when you add or remove variables after adding a frozen constraint. This is because existing CSRConstraints record the number of active variables (matrix columns) in ashapeparameter, which becomes stale when a new variable is added. When a variable (column) is removed, the CSR indices change and have to be rebuilt.This PR makes it so CSR representations are updated when variables are added and removed. Especially in the case of adding variables, this is very low-impact, since the actual CSR data doesn't change; only the "shape" does.
I guess there is an argument to be made for a stricter kind of behavior: that no variables may be added or removed after any CSRConstraint has been added to the model. I believe that may be overly strict since the solution in this PR seems to work fine, but happy to discuss the alternative if anyone else has any insights. In any case, though, if the intention is that no variables may be added or removed when CSRConstaints are present, then this should be enforced properly at variable addition/removal. (Currently, you just get an obscure matrix alignment error when passing the model to a solver.)
Code and tests were written with the help of Claude Code.
Changes proposed in this Pull Request
to_matrixin case any variables were added and matrix shape is stale.Checklist
AGENTS.md).doc.doc/release_notes.rstof the upcoming release is included.