Repository navigation
test(sinks): dense_columns must not edit the model it projects - #427
Merged
Merged
Conversation
`dense_columns` replaces our infinities with the solver's, in place — safe today only because every vector it returns is freshly allocated by the scatter that builds it. The moment any of them becomes a zero-copy view of `cols`, `nan_to_num(copy=False)` rewrites the *built model* to suit whichever solver asked last, and a second `solve()` on another solver reads bounds the first already changed. Nothing in the suite noticed: one solve per model, and the frames are never read again afterwards. So the property is asserted outright — two projections of one model at different infinities, the frame unchanged between them, and the second vector carrying its own spelling rather than the first's. Found while making `cols` positional, where the vectors stop being copies. That change is not this one and did not land; the trap it uncovered is real either way.
FBumann
added
the
area:engine
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
FBumann
added a commit
that referenced
this pull request
Jul 31, 2026
FBumann
added a commit
that referenced
this pull request
Jul 31, 2026
`test_relational.py` is the only conflict and it is textual — main appended `test_dense_columns_does_not_edit_the_model_it_projects` (#427) where this branch appended the positional-cols test. Both kept. They belong together. #427 pins that the projection must not rewrite the model in place, and it is *this* change that makes that reachable: with `cols` positional, `dense_columns` hands out views of the frame rather than the scatter's fresh arrays, so `nan_to_num` had to stop being in-place. The guard was written against a hazard this branch creates. 754 passed, 23 of them Gurobi's, ruff + pyrefly + mkdocs --strict clean.
FBumann
added a commit
that referenced
this pull request
Sep 10, 2026
… the head of energy-models/mathspec#421 (#1565) > **Prompt:** Lets check if lpspec needs to adjust anything after we merge the stack below energy-models/mathspec#421 — Do a Draft pr pinning the head of #421 > [!NOTE] > The following content was generated by AI. The pin is now the release tag `v0.0.0-alpha.82`, which carries the whole five-PR stack for math-spec's issue #401 (#415, #416, #418, #420, #421 — the last squash commit, a2dcee7, is an ancestor of the tag). It replaces the commit `37f8b5136f8433a56653c1954d7ffe770902dd40`, the head of [energy-models/mathspec#421](energy-models/mathspec#421), that this PR pinned while no release contained the stack. lpspec needs no code change. Nothing it imports from math-spec moved; the one consumer-visible change is that math-spec's Markdown typesetter now escapes a `description:` the way its LaTeX and Typst formats already did (`_`, `[`, `]`, `*` set as characters, a name in backticks as a code span). So this PR is: - the pin, in `pyproject.toml` and the three math-spec lines of `uv.lock` - the regenerated pages (`tools.gallery_math`, `tools.ladder`): every changed line is an escape such as `'bp\_x'` or `\[0, 1\]` - two descriptions written plain, in `examples/ports/pypsa_cvar.yaml` and `examples/ports/pypsa_losses.yaml`, because their `*and*` now sets as literal asterisks (and never set as emphasis in LaTeX or Typst); the same two lines inside the YAML block the gallery pages embed verbatim, which no tool writes Against the commit pin the full suite on `main` at 2928e08 was 3754 passed, 251 skipped, 1 xfailed, 2 failed, the two failures being the stale generated pages this PR regenerates. <details><summary>Verified at the tag</summary> - `uv lock` resolves math-spec to `v0.0.0a82` (commit 140ec94e); locked with uv 0.12.11 so the lock diff is the three math-spec lines only (older uv versions rewrite marker annotations across the file) - `uv sync --all-extras` - `uv run python -m tools.gallery_math`, `uv run python -m tools.ladder`, `uv run python -m tools.constructs`: no page changed against the commit pin, so the tag's tree is the pinned commit's tree for everything lpspec renders - `uv run pytest -q -n auto tests/test_models_gallery.py tests/test_pypsa_ladder_page.py tests/test_typeset_gallery.py tests/test_docs_site.py tests/test_ports.py`: 612 passed, 14 skipped (the typst binary, and the MILP ports that record no duals) - `ruff check .`, `ruff format --check .`, `pyrefly check` on both configs: clean - Left alone on purpose: the `*and*` in the comment of `examples/symbols/transport.yaml` and the bold sentence in the hand-written prose of `docs/examples/pypsa_losses.md`, neither being a description - Not run here: the full suite under pixi, and the typst-backed typeset tests </details> <details><summary>The next math-spec release needs a follow-up in lpspec</summary> math-spec's `main` already carries #427 (`refactor(program): a variable's domain is called domain in the program`), which renames `VariableDeclaration.variable_type` to `domain`. It is **not** in alpha.82, so this pin is unaffected. lpspec reads `variable_type` in `src/lpspec/linopy/builder.py` at three sites (two in the variable attach, one in the discrete-variable listing before the solve), and the next math-spec release will need a follow-up in lpspec that renames those reads. Not done here. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_019d4fAY3RikKY317uerZ2Hc https://claude.ai/code/session_017tCiQ1MTt4jG8X3qNEGNvZ --------- Co-authored-by: Claude <noreply@anthropic.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
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.
A guard for a trap that is live today and that nothing in the suite would notice.
The hazard
ModelTables.dense_columnsreplaces our±infwith the solver's spelling of infinity in place:That is safe only because every vector it returns is freshly allocated —
_scatteredbuilds each one withnp.fulland writes values into it. The mutation lands on a private array.It stops being safe the moment any of those becomes a view of
cols. Then the projection edits the built model, and a secondsolve()with a different solver reads bounds the first solver's call already rewrote. Withhighsandgurobiboth shipping since #418, two solves of one model is now an ordinary thing to do.Why the suite missed it
Every test solves once and never reads the frames afterwards. The bug needs two projections at different infinities against one model, which nothing did.
So the test does exactly that: project at
1e30, check the frame is untouched, project again at1e100, and check the second vector carries its own infinity rather than the first's.It passes on
mainas it stands. It fails the instant the projection stops allocating.Where it came from
Found while making
colspositional — dropping itscolcolumn so a row's position is its index, which turns those three vectors into zero-copy views. That change measured 8–16% off peak on the write path and 13–92% onto the build, so it did not land and I'll come back to it from the other end. The trap it uncovered does not depend on it.🤖 Generated with Claude Code